riscv: spacemit: Add PCIe RC controller support for K3

PATCH Superseded Inochi Amaoto 6 participants 23 messages May 17 – Jul 3

Subsystems (6)

Device tree bindings PCI RISC-V PCI native host bridge and endpoint drivers +2 more subsystems

lore

Share & tools

lore

Inochi Amaoto

author

2026-05-17 01:49 UTC #0 lore raw

v2 cover riscv: spacemit: Add PCIe RC controller support for K3

The PCIe controller on Spacemit K3 is almost a standard Synopsys Designware PCIe IP with extra control and external MSI controller (IMSIC).

Add binding and driver support for PCIe RC controller support on K3.

Changed from v1:
patch 1:
1. Remove post_init callback.
patch 2:
1. Fix devm_kmalloc_array arguments order and error check. patch 3:
1. Apply Rob's tag.
2. Change check from oneOf to anyOf.
3. Fix spelling mistakes in commit message.
patch 4:
1. Add comment about PHY handle.
2. Remove interrupt "app".
3. Fix spelling mistakes in commit message.
patch 5:
1. Remove all logic related to link up interrupt.
2. Clean up all unused macros.
3. Fix devm_kmalloc_array arguments order and error check.
4. Remove unnecessary start_link logic.
5. Make all magic number as a macro.
6. Fix spelling mistakes in code and commit message.

Inochi Amaoto (5):
PCI: spacemit-k1: Add device data support
PCI: spacemit-k1: Add multiple PHY handles support
dt-bindings: PCI: snps,dw-pcie: Add msi-parent for MSI handle check
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

 .../devicetree/bindings/pci/snps,dw-pcie.yaml |   7 +-
 .../bindings/pci/spacemit,k3-pcie-host.yaml   | 135 +++++++++++
 drivers/pci/controller/dwc/Kconfig            |   4 +-
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 210 +++++++++++++++++-
 4 files changed, 344 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml

--
2.54.0

Inochi Amaoto

author

2026-05-17 01:49 UTC #1 lore raw

PATCH v2 1/5 PCI: spacemit-k1: Add device data support

To reuse the K1 PCIe driver logic for K3 PCIe controller, add device data to handle the K1 specific logic and make room for the incoming logic for K3.

Signed-off-by Inochi Amaoto <[email protected]>

---
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 25 ++++++++++++++++---
 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-spacemit-k1.c +21 -4

index be20a520255b..1b519d49dcc0 100644
@@ -57,6 +57,12 @@ struct k1_pcie {
 	u32 pmu_off;
 };
 
+struct k1_pcie_device_data {
+	const struct dw_pcie_host_ops *host_ops;
+	const struct dw_pcie_ops *ops;
+	int (*parse_port)(struct k1_pcie *k1);
+};
+
 #define to_k1_pcie(dw_pcie) \
 		platform_get_drvdata(to_platform_device((dw_pcie)->dev))

@@ -278,10 +284,15 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
 
 static int k1_pcie_probe(struct platform_device *pdev)
 {
+	const struct k1_pcie_device_data *data;
 	struct device *dev = &pdev->dev;
 	struct k1_pcie *k1;
 	int ret;
 
+	data = device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
 	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
 	if (!k1)
 		return -ENOMEM;

@@ -299,11 +310,11 @@ static int k1_pcie_probe(struct platform_device *pdev)
 				     "failed to map \"link\" registers\n");
 
 	k1->pci.dev = dev;
-	k1->pci.ops = &k1_pcie_ops;
+	k1->pci.ops = data->ops;
 	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
 	dw_pcie_cap_set(&k1->pci, REQ_RES);
 
-	k1->pci.pp.ops = &k1_pcie_host_ops;
+	k1->pci.pp.ops = data->host_ops;
 
 	/* Hold the PHY in reset until we start the link */
 	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,

@@ -320,7 +331,7 @@ static int k1_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, k1);
 
-	ret = k1_pcie_parse_port(k1);
+	ret = data->parse_port(k1);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to parse root port\n");

@@ -338,8 +349,14 @@ static void k1_pcie_remove(struct platform_device *pdev)
 	dw_pcie_host_deinit(&k1->pci.pp);
 }
 
+static const struct k1_pcie_device_data k1_pcie_device_data = {
+	.host_ops	= &k1_pcie_host_ops,
+	.ops		= &k1_pcie_ops,
+	.parse_port	= k1_pcie_parse_port,
+};
+
 static const struct of_device_id k1_pcie_of_match_table[] = {
-	{ .compatible = "spacemit,k1-pcie", },
+	{ .compatible = "spacemit,k1-pcie", .data = &k1_pcie_device_data},
 	{ }
 };

Signature · 1 line

2.54.0

Inochi Amaoto

author

2026-05-17 01:49 UTC #2 lore raw

PATCH v2 2/5 PCI: spacemit-k1: Add multiple PHY handles support

In reply to Inochi Amaoto

The PCIe controller on Spacemit K3 may use multiple PHYs at the same time. The feature is not support by the current driver. So extend the PHY definition to support multiple PHY handles.

Signed-off-by Inochi Amaoto <[email protected]>

---
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-spacemit-k1.c +12 -4

index 1b519d49dcc0..7f6f1df31cd8 100644
@@ -51,7 +51,8 @@
 
 struct k1_pcie {
 	struct dw_pcie pci;
-	struct phy *phy;
+	struct phy **phy;
+	int phy_count;
 	void __iomem *link;
 	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
 	u32 pmu_off;

@@ -171,7 +172,7 @@ static int k1_pcie_init(struct dw_pcie_rp *pp)
 	 */
 	regmap_set_bits(k1->pmu, reset_ctrl, DEVICE_TYPE_RC | PCIE_AUX_PWR_DET);
 
-	ret = phy_init(k1->phy);
+	ret = phy_init(k1->phy[0]);
 	if (ret) {
 		k1_pcie_disable_resources(k1);

@@ -191,12 +192,14 @@ static void k1_pcie_deinit(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct k1_pcie *k1 = to_k1_pcie(pci);
+	int i;
 
 	/* Assert fundamental reset (drive PERST# low) */
 	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
 			PCIE_RC_PERST);
 
-	phy_exit(k1->phy);
+	for (i = 0; i < k1->phy_count; i++)
+		phy_exit(k1->phy[i]);
 
 	k1_pcie_disable_resources(k1);
 }

@@ -277,7 +280,12 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 
-	k1->phy = phy;
+	k1->phy = devm_kmalloc_array(dev, 1, sizeof(*k1->phy), GFP_KERNEL);
+	if (!k1->phy)
+		return -ENOMEM;
+
+	k1->phy[0] = phy;
+	k1->phy_count = 1;
 
 	return 0;
 }

Signature · 1 line

2.54.0

Andy Shevchenko

2026-05-17 08:07 UTC #3 lore raw

Re: [PATCH v2 2/5] PCI: spacemit-k1: Add multiple PHY handles support

Quoted 3 lines · Inochi Amaoto

The PCIe controller on Spacemit K3 may use multiple PHYs at the same time. The feature is not support by the current driver. So extend the PHY definition to support multiple PHY handles.

...

drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 283

Quoted 1 line

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	k1->phy = devm_kmalloc_array(dev, 1, sizeof(*k1->phy), GFP_KERNEL);

Hmm... What's the point in having _array(1) ? devm_kmalloc() should suffice.

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+	if (!k1->phy)
+		return -ENOMEM;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Inochi Amaoto

author

2026-05-18 01:18 UTC #4 lore raw

Re: [PATCH v2 2/5] PCI: spacemit-k1: Add multiple PHY handles support

Quoted 11 lines · Andy Shevchenko

Quoted 3 lines · Inochi Amaoto
The PCIe controller on Spacemit K3 may use multiple PHYs at the same time. The feature is not support by the current driver. So extend the PHY definition to support multiple PHY handles.
...
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 283
Quoted 1 line
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	k1->phy = devm_kmalloc_array(dev, 1, sizeof(*k1->phy), GFP_KERNEL);
Hmm... What's the point in having _array(1) ? devm_kmalloc() should suffice.

Yes, you are right, I forgot it is "struct phy *", so the devm_kmalloc() is enough, will fix in the next version.

Regards,
Inochi

Quoted 8 lines

Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+	if (!k1->phy)
+		return -ENOMEM;
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Alex Elder

2026-06-09 16:11 UTC #5 lore raw

Re: [PATCH v2 2/5] PCI: spacemit-k1: Add multiple PHY handles support

In reply to Inochi Amaoto

Quoted 14 lines · Inochi Amaoto

The PCIe controller on Spacemit K3 may use multiple PHYs at the same time. The feature is not support by the current driver. So extend the PHY definition to support multiple PHY handles.
Signed-off-by Inochi Amaoto <[email protected]>
---
  drivers/pci/controller/dwc/pcie-spacemit-k1.c | 16 ++++++++++++----
  1 file changed, 12 insertions(+), 4 deletions(-)
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0
index 1b519d49dcc0..7f6f1df31cd8 100644

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 54

Quoted 5 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -1

@@ -51,7 +51,8 @@
 
  struct k1_pcie {
  	struct dw_pcie pci;
-	struct phy *phy;
+	struct phy **phy;

I haven't looked further at this series yet, but do you have any interest in making this a flexible array of pointers (counted_by(phy_count))?

drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_init(struct dw_pcie_rp *pp) · line 175

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -1

+	int phy_count;
  	void __iomem *link;
  	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
  	u32 pmu_off;

@@ -171,7 +172,7 @@ static int k1_pcie_init(struct dw_pcie_rp *pp)
  	 */
  	regmap_set_bits(k1->pmu, reset_ctrl, DEVICE_TYPE_RC | PCIE_AUX_PWR_DET);
 
-	ret = phy_init(k1->phy);
+	ret = phy_init(k1->phy[0]);

If you're going to have an array you should probably put in place a loop to initialize all phy_count elements of the array here (as is done for phy_exit(), below).

Quoted 20 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -1

  	if (ret) {
  		k1_pcie_disable_resources(k1);

@@ -191,12 +192,14 @@ static void k1_pcie_deinit(struct dw_pcie_rp *pp)
  {
  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
  	struct k1_pcie *k1 = to_k1_pcie(pci);
+	int i;
 
  	/* Assert fundamental reset (drive PERST# low) */
  	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
  			PCIE_RC_PERST);
 
-	phy_exit(k1->phy);
+	for (i = 0; i < k1->phy_count; i++)
+		phy_exit(k1->phy[i]);
 
  	k1_pcie_disable_resources(k1);
  }

drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 281

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0

@@ -277,7 +280,12 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
  	if (IS_ERR(phy))
  		return PTR_ERR(phy);

Andy said something different about this.

But it seems to me you are intentionally allocating an *array* of PHY pointers, which happens to have only one entry. But your objective is that you want to allocate an array of them so you can support more than just one PHY, right?

drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 283

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -1
-	k1->phy = phy;
+	k1->phy = devm_kmalloc_array(dev, 1, sizeof(*k1->phy), GFP_KERNEL);

Probably should use the kzalloc variant.

drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 287

Quoted 4 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +4 -0
+	if (!k1->phy)
+		return -ENOMEM;
+
+	k1->phy[0] = phy;

I think what's wrong is the above assignment.

If you are truly allocating an array of PHY *pointers*, then you need to also allocate the phy structure that each entry in the array points to.

The above assignment is erroneously assigning the first entry in the array to point to the array, and not a new entry.

-Alex

Quoted 4 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	k1->phy_count = 1;
 
  	return 0;
  }

Inochi Amaoto

author

2026-05-17 01:49 UTC #6 lore raw

PATCH v2 3/5 dt-bindings: PCI: snps,dw-pcie: Add msi-parent for MSI handle check

In reply to Inochi Amaoto

The IMSIC device on RISC-V based system does not require ID remapping for MSI. So this device only needs "msi-parent" property for IMSIC-based SoC, and the "msi-map" is not a necessary property.

Add new condition for MSI handling on IMSIC based SoC.

Signed-off-by Inochi Amaoto <[email protected]>

Acked-by Rob Herring (Arm) <[email protected]>

---
 Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml +5 -2

index b3216141881c..91bbbc8924f6 100644
@@ -27,8 +27,11 @@ allOf:
   - $ref: /schemas/pci/snps,dw-pcie-common.yaml#
   - if:
       not:
-        required:
-          - msi-map
+        anyOf:
+          - required:
+              - msi-map
+          - required:
+              - msi-parent
     then:
       properties:
         interrupt-names:

Signature · 1 line

2.54.0

Inochi Amaoto

author

2026-05-17 01:49 UTC #7 lore raw

PATCH v2 4/5 dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

In reply to Inochi Amaoto

Add binding support for the PCIe controller on the SpacemiT K3 SoC. This controller is almost a standard Synopsys DesignWare PCIe IP, with some extra link and reset state control.

Signed-off-by Inochi Amaoto <[email protected]>

---
 .../bindings/pci/spacemit,k3-pcie-host.yaml   | 135 ++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +135 -0

new file mode 100644
index 000000000000..46147a37a9ce
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/spacemit,k3-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K3 PCI Express Host Controller
+
+maintainers:
+  - Inochi Amaoto <[email protected]>
+
+description:
+  The SpacemiT K3 SoC PCIe host controller is based on the Synopsys
+  DesignWare PCIe IP. The controller uses the external MSI interrupt
+  controller.
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    const: spacemit,k3-pcie
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: Data Bus Interface (DBI) shadow registers
+      - description: ATU address space
+      - description: PCIe configuration space
+      - description: Link control registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: dbi2
+      - const: atu
+      - const: config
+      - const: link
+
+  clocks:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) clock
+      - description: DWC PCIe application AXI-bus master interface clock
+      - description: DWC PCIe application AXI-bus slave interface clock
+
+  clock-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  resets:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) reset
+      - description: DWC PCIe application AXI-bus master interface reset
+      - description: DWC PCIe application AXI-bus slave interface reset
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+
+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6
+
+  spacemit,apmu:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      A phandle that refers to the APMU system controller, whose regmap is
+      used in managing resets and link state, along with and offset of its
+      reset control register.
+    items:
+      - items:
+          - description: phandle to APMU system controller
+          - description: register offset
+
+required:
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - msi-parent
+  - spacemit,apmu
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie@80000000 {
+        compatible = "spacemit,k3-pcie";
+        reg = <0x0  0x80000000 0x0 0x00001000>,
+              <0x0  0x80100000 0x0 0x00001000>,
+              <0x0  0x80300000 0x0 0x00003f20>,
+              <0x11 0x00000000 0x0 0x00010000>,
+              <0x0  0x82900000 0x0 0x00001000>;
+        reg-names = "dbi", "dbi2", "atu", "config", "link";
+        device_type = "pci";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        clocks = <&syscon_apmu 89>,
+                 <&syscon_apmu 56>,
+                 <&syscon_apmu 57>;
+        clock-names = "dbi", "mstr", "slv";
+        msi-parent = <&simsic>;
+        ranges = <0x01000000 0x00 0x00010000 0x11 0x00010000 0x0 0x00100000>,
+                 <0x02000000 0x0  0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
+                 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
+        resets = <&syscon_apmu 76>,
+                 <&syscon_apmu 78>,
+                 <&syscon_apmu 77>;
+        reset-names = "dbi", "mstr", "slv";
+        linux,pci-domain = <0>;
+        spacemit,apmu = <&syscon_apmu 0x1f0>;
+      };
+    };
+

Signature · 1 line

2.54.0

[email protected]

AI review

2026-05-17 02:48 UTC #8 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] The binding completely omits the `interrupts` and `interrupt-names` properties for the host…

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding completely omits the interrupts and interrupt-names properties for the host controller.
- [Medium] The schema omits standard legacy INTx interrupt routing properties (#interrupt-cells, interrupt-map, interrupt-map-mask).
- [Low] The phy-names property lacks an items definition to restrict valid string values.
--

commit 7b8ba34456e71d6a9cf1028caa5504d64e8f58ba
Author: Inochi Amaoto <[email protected]>

dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

This commit adds device tree binding support for the PCIe controller on the SpacemiT K3 SoC. The controller is based on the Synopsys DesignWare PCIe IP with some extra link and reset state control.

Quoted 5 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 0000000000000..46147a37a9ce6

[ ... ]

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 66

Quoted 8 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+

The driver implementation reads and clears interrupt status registers for host-level events, and references registers for receiving INTx events from downstream devices.

Should this binding include interrupts and interrupt-names properties to document the host's interrupt routing?

Additionally, should standard legacy INTx interrupt routing properties like #interrupt-cells, interrupt-map, and interrupt-map-mask be included in the schema and example to support downstream devices that do not use MSI?

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 77

Quoted 11 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +11 -0

+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6

This isn't a bug, but the phy-names property lacks an items definition to restrict valid string values. The schema allows any arbitrary string here.

Since the driver retrieves PHYs by index rather than name, is this property necessary, or should the expected string items be defined?

[ ... ]

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

Inochi Amaoto

author

2026-05-17 04:39 UTC #9 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Quoted 34 lines · On Sun, May 17, 2026 at 02:48:47AM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding completely omits the interrupts and interrupt-names properties for the host controller.
- [Medium] The schema omits standard legacy INTx interrupt routing properties (#interrupt-cells, interrupt-map, interrupt-map-mask).
- [Low] The phy-names property lacks an items definition to restrict valid string values.
--
commit 7b8ba34456e71d6a9cf1028caa5504d64e8f58ba
Author: Inochi Amaoto <[email protected]>
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
This commit adds device tree binding support for the PCIe controller on the SpacemiT K3 SoC. The controller is based on the Synopsys DesignWare PCIe IP with some extra link and reset state control.
Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 0000000000000..46147a37a9ce6
[ ... ]
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 66
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+
The driver implementation reads and clears interrupt status registers for host-level events, and references registers for receiving INTx events from downstream devices.

Quoted 7 lines

Should this binding include interrupts and interrupt-names properties to document the host's interrupt routing?
Additionally, should standard legacy INTx interrupt routing properties like #interrupt-cells, interrupt-map, and interrupt-map-mask be included in the schema and example to support downstream devices that do not use MSI?

The driver just does the interrupt cleanup as it requires, and no additional interrupt is needed

Quoted 12 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 77
Quoted 11 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +11 -0
+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6

Quoted 6 lines

This isn't a bug, but the phy-names property lacks an items definition to restrict valid string values. The schema allows any arbitrary string here.
Since the driver retrieves PHYs by index rather than name, is this property necessary, or should the expected string items be defined?

The driver just use index, and the name follow generic DesignWare binding.

Quoted 4 lines

[ ... ]

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

Regards,
Inochi

Rob Herring

maintainer

2026-06-01 22:42 UTC #10 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Quoted 38 lines · Inochi Amaoto

Quoted 37 lines · On Sun, May 17, 2026 at 02:48:47AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding completely omits the interrupts and interrupt-names properties for the host controller.
- [Medium] The schema omits standard legacy INTx interrupt routing properties (#interrupt-cells, interrupt-map, interrupt-map-mask).
- [Low] The phy-names property lacks an items definition to restrict valid string values.
--
commit 7b8ba34456e71d6a9cf1028caa5504d64e8f58ba
Author: Inochi Amaoto <[email protected]>
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
This commit adds device tree binding support for the PCIe controller on the SpacemiT K3 SoC. The controller is based on the Synopsys DesignWare PCIe IP with some extra link and reset state control.
Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 0000000000000..46147a37a9ce6
[ ... ]
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 66
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+
The driver implementation reads and clears interrupt status registers for host-level events, and references registers for receiving INTx events from downstream devices.
Should this binding include interrupts and interrupt-names properties to document the host's interrupt routing?

Quoted 7 lines

Quoted 4 lines
Additionally, should standard legacy INTx interrupt routing properties like #interrupt-cells, interrupt-map, and interrupt-map-mask be included in the schema and example to support downstream devices that do not use MSI?
The driver just does the interrupt cleanup as it requires, and no additional interrupt is needed

Unless there are no interrupts and only MSIs are supported, then you need to define 'interrupts' property at least. The common schema only defines all possible names, but not which ones are used and the order.

Rob

Inochi Amaoto

author

2026-06-03 09:24 UTC #11 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Quoted 44 lines · Rob Herring

Quoted 43 lines · Inochi Amaoto
Quoted 41 lines · On Sun, May 17, 2026 at 02:48:47AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding completely omits the interrupts and interrupt-names properties for the host controller.
- [Medium] The schema omits standard legacy INTx interrupt routing properties (#interrupt-cells, interrupt-map, interrupt-map-mask).
- [Low] The phy-names property lacks an items definition to restrict valid string values.
--
commit 7b8ba34456e71d6a9cf1028caa5504d64e8f58ba
Author: Inochi Amaoto <[email protected]>
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
This commit adds device tree binding support for the PCIe controller on the SpacemiT K3 SoC. The controller is based on the Synopsys DesignWare PCIe IP with some extra link and reset state control.
Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 0000000000000..46147a37a9ce6
[ ... ]
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 66
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+
The driver implementation reads and clears interrupt status registers for host-level events, and references registers for receiving INTx events from downstream devices.
Should this binding include interrupts and interrupt-names properties to document the host's interrupt routing?
Additionally, should standard legacy INTx interrupt routing properties like #interrupt-cells, interrupt-map, and interrupt-map-mask be included in the schema and example to support downstream devices that do not use MSI?

Quoted 8 lines

Quoted 2 lines
The driver just does the interrupt cleanup as it requires, and no additional interrupt is needed
Unless there are no interrupts and only MSIs are supported, then you need to define 'interrupts' property at least. The common schema only defines all possible names, but not which ones are used and the order.
Rob

Yes, currently there is no interrupts that should be enabled in Linux, and only MSI is supported without INTx. So I think it is fine to removed the interrupt properity.

Regards,
Inochi

Manivannan Sadhasivam

maintainer

2026-06-09 14:03 UTC #12 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

In reply to Inochi Amaoto

Quoted 5 lines · Inochi Amaoto

Add binding support for the PCIe controller on the SpacemiT K3 SoC. This controller is almost a standard Synopsys DesignWare PCIe IP, with some extra link and reset state control.
Signed-off-by Inochi Amaoto <[email protected]>

Why can't you reuse the existing spacemit,k1-pcie-host.yaml binding? I see very few differences which could be added using conditionals. Also, this binding defines the PHY property in the controller node, which is a way backwards as we now prefer to define these in Root Port node as spacemit,k1-pcie-host.yaml does.

- Mani

Quoted 149 lines

---
 .../bindings/pci/spacemit,k3-pcie-host.yaml   | 135 ++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +135 -0

new file mode 100644
index 000000000000..46147a37a9ce
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/spacemit,k3-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K3 PCI Express Host Controller
+
+maintainers:
+  - Inochi Amaoto <[email protected]>
+
+description:
+  The SpacemiT K3 SoC PCIe host controller is based on the Synopsys
+  DesignWare PCIe IP. The controller uses the external MSI interrupt
+  controller.
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    const: spacemit,k3-pcie
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: Data Bus Interface (DBI) shadow registers
+      - description: ATU address space
+      - description: PCIe configuration space
+      - description: Link control registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: dbi2
+      - const: atu
+      - const: config
+      - const: link
+
+  clocks:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) clock
+      - description: DWC PCIe application AXI-bus master interface clock
+      - description: DWC PCIe application AXI-bus slave interface clock
+
+  clock-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  resets:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) reset
+      - description: DWC PCIe application AXI-bus master interface reset
+      - description: DWC PCIe application AXI-bus slave interface reset
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
+
+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6
+
+  spacemit,apmu:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      A phandle that refers to the APMU system controller, whose regmap is
+      used in managing resets and link state, along with and offset of its
+      reset control register.
+    items:
+      - items:
+          - description: phandle to APMU system controller
+          - description: register offset
+
+required:
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - msi-parent
+  - spacemit,apmu
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie@80000000 {
+        compatible = "spacemit,k3-pcie";
+        reg = <0x0  0x80000000 0x0 0x00001000>,
+              <0x0  0x80100000 0x0 0x00001000>,
+              <0x0  0x80300000 0x0 0x00003f20>,
+              <0x11 0x00000000 0x0 0x00010000>,
+              <0x0  0x82900000 0x0 0x00001000>;
+        reg-names = "dbi", "dbi2", "atu", "config", "link";
+        device_type = "pci";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        clocks = <&syscon_apmu 89>,
+                 <&syscon_apmu 56>,
+                 <&syscon_apmu 57>;
+        clock-names = "dbi", "mstr", "slv";
+        msi-parent = <&simsic>;
+        ranges = <0x01000000 0x00 0x00010000 0x11 0x00010000 0x0 0x00100000>,
+                 <0x02000000 0x0  0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
+                 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
+        resets = <&syscon_apmu 76>,
+                 <&syscon_apmu 78>,
+                 <&syscon_apmu 77>;
+        reset-names = "dbi", "mstr", "slv";
+        linux,pci-domain = <0>;
+        spacemit,apmu = <&syscon_apmu 0x1f0>;
+      };
+    };
+

Signature · 1 line

2.54.0

Signature · 1 line

மணிவண்ணன் சதாசிவம்

Alex Elder

2026-06-09 16:11 UTC #13 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Quoted 11 lines · Manivannan Sadhasivam

Quoted 5 lines · Inochi Amaoto
Add binding support for the PCIe controller on the SpacemiT K3 SoC. This controller is almost a standard Synopsys DesignWare PCIe IP, with some extra link and reset state control.
Signed-off-by Inochi Amaoto <[email protected]>
Why can't you reuse the existing spacemit,k1-pcie-host.yaml binding? I see very few differences which could be added using conditionals. Also, this binding defines the PHY property in the controller node, which is a way backwards as we now prefer to define these in Root Port node as spacemit,k1-pcie-host.yaml does.

I agree. I have another couple of comments below, including several things that just point out what's the same and what's different.

-Alex

Quoted 8 lines

- Mani
Quoted 5 lines
---
  .../bindings/pci/spacemit,k3-pcie-host.yaml   | 135 ++++++++++++++++++
  1 file changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml

Quoted 16 lines

Quoted 6 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 000000000000..46147a37a9ce
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 10
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +10 -0
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/spacemit,k3-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K3 PCI Express Host Controller
+
+maintainers:
+  - Inochi Amaoto <[email protected]>

If you would like to maintain the (common) SpacemiT K1 host controller binding I'd be OK with that.

Quoted 10 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +10 -0

+
+description:
+  The SpacemiT K3 SoC PCIe host controller is based on the Synopsys
+  DesignWare PCIe IP. The controller uses the external MSI interrupt
+  controller.
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+

Quoted 8 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 28
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+properties:
+  compatible:
+    const: spacemit,k3-pcie
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: Data Bus Interface (DBI) shadow registers

The above is new (different from K1), as is "dbi2" as its name.

Quoted 5 lines

Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +5 -0
+      - description: ATU address space
+      - description: PCIe configuration space
+      - description: Link control registers
+
+  reg-names:

Quoted 8 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 41
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+    items:
+      - const: dbi
+      - const: dbi2
+      - const: atu
+      - const: config
+      - const: link
+
+  clocks:

Clocks and resets are the same as K1.

Quoted 16 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +16 -0

+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) clock
+      - description: DWC PCIe application AXI-bus master interface clock
+      - description: DWC PCIe application AXI-bus slave interface clock
+
+  clock-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  resets:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) reset
+      - description: DWC PCIe application AXI-bus master interface reset
+      - description: DWC PCIe application AXI-bus slave interface reset

Quoted 8 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 65
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true

msi-parent and phys are not present in the K1 binding.

Quoted 5 lines

Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +5 -0
+
+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be

Quoted 8 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 78
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6
+

The following property is the same as K1.

Quoted 61 lines

Quoted 60 lines

Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +57 -0

+  spacemit,apmu:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      A phandle that refers to the APMU system controller, whose regmap is
+      used in managing resets and link state, along with and offset of its
+      reset control register.
+    items:
+      - items:
+          - description: phandle to APMU system controller
+          - description: register offset
+
+required:
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - msi-parent
+  - spacemit,apmu
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie@80000000 {
+        compatible = "spacemit,k3-pcie";
+        reg = <0x0  0x80000000 0x0 0x00001000>,
+              <0x0  0x80100000 0x0 0x00001000>,
+              <0x0  0x80300000 0x0 0x00003f20>,
+              <0x11 0x00000000 0x0 0x00010000>,
+              <0x0  0x82900000 0x0 0x00001000>;
+        reg-names = "dbi", "dbi2", "atu", "config", "link";
+        device_type = "pci";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        clocks = <&syscon_apmu 89>,
+                 <&syscon_apmu 56>,
+                 <&syscon_apmu 57>;
+        clock-names = "dbi", "mstr", "slv";
+        msi-parent = <&simsic>;
+        ranges = <0x01000000 0x00 0x00010000 0x11 0x00010000 0x0 0x00100000>,
+                 <0x02000000 0x0  0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
+                 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
+        resets = <&syscon_apmu 76>,
+                 <&syscon_apmu 78>,
+                 <&syscon_apmu 77>;
+        reset-names = "dbi", "mstr", "slv";
+        linux,pci-domain = <0>;
+        spacemit,apmu = <&syscon_apmu 0x1f0>;
+      };
+    };
+

Signature · 1 line

2.54.0

Inochi Amaoto

author

2026-07-03 01:51 UTC #14 lore raw

Re: [PATCH v2 4/5] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller

Quoted 13 lines · Alex Elder

Quoted 11 lines · Manivannan Sadhasivam
Quoted 5 lines · Inochi Amaoto
Add binding support for the PCIe controller on the SpacemiT K3 SoC. This controller is almost a standard Synopsys DesignWare PCIe IP, with some extra link and reset state control.
Signed-off-by Inochi Amaoto <[email protected]>
Why can't you reuse the existing spacemit,k1-pcie-host.yaml binding? I see very few differences which could be added using conditionals. Also, this binding defines the PHY property in the controller node, which is a way backwards as we now prefer to define these in Root Port node as spacemit,k1-pcie-host.yaml does.

Quoted 5 lines

I agree. I have another couple of comments below, including several things that just point out what's the same and what's different.
-Alex

A good caught, I have confirmed, they share the same topology so it can be reused.

Regards,
Inochi

Quoted 170 lines

Quoted 8 lines
- Mani
Quoted 5 lines
---
  .../bindings/pci/spacemit,k3-pcie-host.yaml   | 135 ++++++++++++++++++
  1 file changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml
Quoted 16 lines
Quoted 6 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +0 -0
new file mode 100644
index 000000000000..46147a37a9ce
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 10
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +10 -0
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/spacemit,k3-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K3 PCI Express Host Controller
+
+maintainers:
+  - Inochi Amaoto <[email protected]>
If you would like to maintain the (common) SpacemiT K1 host controller binding I'd be OK with that.
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +10 -0
+
+description:
+  The SpacemiT K3 SoC PCIe host controller is based on the Synopsys
+  DesignWare PCIe IP. The controller uses the external MSI interrupt
+  controller.
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 28
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+properties:
+  compatible:
+    const: spacemit,k3-pcie
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: Data Bus Interface (DBI) shadow registers
The above is new (different from K1), as is "dbi2" as its name.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +5 -0
+      - description: ATU address space
+      - description: PCIe configuration space
+      - description: Link control registers
+
+  reg-names:
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 41
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+    items:
+      - const: dbi
+      - const: dbi2
+      - const: atu
+      - const: config
+      - const: link
+
+  clocks:
Clocks and resets are the same as K1.
Quoted 16 lines
Quoted 16 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +16 -0
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) clock
+      - description: DWC PCIe application AXI-bus master interface clock
+      - description: DWC PCIe application AXI-bus slave interface clock
+
+  clock-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  resets:
+    items:
+      - description: DWC PCIe Data Bus Interface (DBI) reset
+      - description: DWC PCIe application AXI-bus master interface reset
+      - description: DWC PCIe application AXI-bus slave interface reset
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 65
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+
+  msi-parent: true
msi-parent and phys are not present in the K1 binding.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +5 -0
+
+  phys:
+    description:
+      PHY phandle from the Combo PHY, the lane number does not depends
+      on this, since the number of lanes provided by Combo PHY can be
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml line 78
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +8 -0
+      1 or 2.
+    minItems: 1
+    maxItems: 6
+
+  phy-names:
+    minItems: 1
+    maxItems: 6
+
The following property is the same as K1.
Quoted 61 lines
Quoted 60 lines
Documentation/devicetree/bindings/pci/spacemit,k3-pcie-host.yaml new file +57 -0
+  spacemit,apmu:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      A phandle that refers to the APMU system controller, whose regmap is
+      used in managing resets and link state, along with and offset of its
+      reset control register.
+    items:
+      - items:
+          - description: phandle to APMU system controller
+          - description: register offset
+
+required:
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - msi-parent
+  - spacemit,apmu
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie@80000000 {
+        compatible = "spacemit,k3-pcie";
+        reg = <0x0  0x80000000 0x0 0x00001000>,
+              <0x0  0x80100000 0x0 0x00001000>,
+              <0x0  0x80300000 0x0 0x00003f20>,
+              <0x11 0x00000000 0x0 0x00010000>,
+              <0x0  0x82900000 0x0 0x00001000>;
+        reg-names = "dbi", "dbi2", "atu", "config", "link";
+        device_type = "pci";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        clocks = <&syscon_apmu 89>,
+                 <&syscon_apmu 56>,
+                 <&syscon_apmu 57>;
+        clock-names = "dbi", "mstr", "slv";
+        msi-parent = <&simsic>;
+        ranges = <0x01000000 0x00 0x00010000 0x11 0x00010000 0x0 0x00100000>,
+                 <0x02000000 0x0  0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
+                 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
+        resets = <&syscon_apmu 76>,
+                 <&syscon_apmu 78>,
+                 <&syscon_apmu 77>;
+        reset-names = "dbi", "mstr", "slv";
+        linux,pci-domain = <0>;
+        spacemit,apmu = <&syscon_apmu 0x1f0>;
+      };
+    };
+
Signature · 1 line
2.54.0

Inochi Amaoto

author

2026-05-17 01:49 UTC #15 lore raw

PATCH v2 5/5 PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

In reply to Inochi Amaoto

The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.

Add driver to support PCIe controller on Spacemit K3 PCIe.

Signed-off-by Inochi Amaoto <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig            |   4 +-
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 169 ++++++++++++++++++
 2 files changed, 171 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/Kconfig +2 -2

index f2fde13107f2..fae971ecd876 100644
@@ -439,7 +439,7 @@ config PCIE_SOPHGO_DW
 	  Sophgo SoCs.
 
 config PCIE_SPACEMIT_K1
-	tristate "SpacemiT K1 PCIe controller (host mode)"
+	tristate "SpacemiT K1/K3 PCIe controller (host mode)"
 	depends on ARCH_SPACEMIT || COMPILE_TEST
 	depends on HAS_IOMEM
 	select PCIE_DW_HOST

@@ -447,7 +447,7 @@ config PCIE_SPACEMIT_K1
 	default ARCH_SPACEMIT
 	help
 	  Enables support for the DesignWare based PCIe controller in
-	  the SpacemiT K1 SoC operating in host mode.  Three controllers
+	  the SpacemiT K1/K3 SoC operating in host mode. Three controllers
 	  are available on the K1 SoC; the first of these shares a PHY
 	  with a USB 3.0 host controller (one or the other can be used).

drivers/pci/controller/dwc/pcie-spacemit-k1.c +169 -0

203 lines not on this page · raw message

Signature · 1 line

2.54.0

[email protected]

AI review

2026-05-17 03:16 UTC #16 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] MMIO registers are accessed before the controller's clocks are enabled, leading to a synchronous…

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] MMIO registers are accessed before the controller's clocks are enabled, leading to a synchronous external abort or system crash.
- [High] Incorrect use of u32_replace_bits() combined with bitwise OR (|=) fails to clear existing bits.
- [High] Missing Root Complex mode configuration for the K3 controller due to a copy-paste error.
- [Medium] A bitmask macro for a completely different field is incorrectly passed as the replacement value to u32_replace_bits(), writing unexpected values.
- [Medium] Asymmetric teardown: k1_pcie_deinit() fails to assert reset on the K3 controller because it uses a reset line the controller is explicitly configured to ignore. --

commit 2826ac7fe9e0d0c0dd248a7fc412ce12f4d7b491
Author: Inochi Amaoto <[email protected]>

PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

This commit adds host controller support for the Spacemit K3 PCIe IP. It introduces K3-specific initialization, PHY enablement, and MSI host setup, hooking these into the existing spacemit-k1 driver.

Quoted 4 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0
index 7f6f1df31cd81..7854d26220a98 100644

[ ... ]

Quoted 1 line

drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0
@@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {

[ ... ]

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+static int k3_pcie_init(struct dw_pcie_rp *pp)
+{

[ ... ]

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 338

Quoted 6 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +6 -0

+	/*
+	 * Put the controller in root complex mode, and indicate that
+	 * Vaux (3.3v) is present.
+	 */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);

Does this code configure the controller as a Root Complex?

The comment mentions putting the controller in root complex mode, but the register write only sets the PERSTN bits. In the K1 initialization, this is done by setting DEVICE_TYPE_RC, which appears to be missing here.

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 342

Quoted 4 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +4 -0

+
+	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
+	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
+			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);

Is the replacement value intended to be GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE?

This passes a bitmask macro for a completely different field into u32_replace_bits(), which shifts the value into the target mask and writes an unexpected value into the register.

Quoted 1 line

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);

[ ... ]

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 365

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
+	val |= u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
+				K3_CACHE_MSTR_AWCACHE_MODE);

Will this clear the existing bits correctly?

The u32_replace_bits() macro returns the fully modified 32-bit word. By using the bitwise OR assignment (|=), the original bits from val are ORed with the new bits, leaving any originally set bits as 1 instead of clearing them.

Quoted 1 line

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);

[ ... ]

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 375

Quoted 3 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0

+static const struct dw_pcie_host_ops k3_pcie_host_ops = {
+	.init		= k3_pcie_init,
+	.deinit		= k1_pcie_deinit,

Can k1_pcie_deinit() successfully assert the reset for the K3 controller?

During initialization, k3_pcie_init() sets PCIE_IGNORE_PERSTN in PCIE_CONTROL_LOGIC. If k1_pcie_deinit() attempts to assert reset using PCIE_RC_PERST, the hardware might ignore it, leaving the device improperly reset during teardown.

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+	.msi_init	= k3_pcie_msi_host_init,
+};

[ ... ]

Quoted 14 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +14 -0

+static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
+				     u32 *status0, u32 *status1, u32 *status2)
+{
+	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	*status1 = readl_relaxed(k1->link + INTR_STATUS);
+	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
+
+	writel_relaxed(*status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	writel_relaxed(*status1, k1->link + INTR_STATUS);
+	writel_relaxed(*status2, k1->link + K3_ADDR_INTR_STATUS1);
+}
+
+static int k3_pcie_parse_port(struct k1_pcie *k1)
+{

[ ... ]

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 425

Quoted 8 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +8 -0

+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
+
+	return 0;
+}

Could accessing these MMIO registers before the clocks are enabled cause a synchronous external abort?

The probe function calls parse_port() before dw_pcie_host_init(). This means k3_pcie_clear_irq_status() performs direct MMIO reads and writes to the k1->link registers before k1_pcie_enable_resources() is called later in k3_pcie_init().

Accessing an unclocked peripheral can cause a bus stall on ARM/RISC-V architectures.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

Inochi Amaoto

author

2026-05-17 04:41 UTC #17 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

Quoted 36 lines · On Sun, May 17, 2026 at 03:16:20AM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] MMIO registers are accessed before the controller's clocks are enabled, leading to a synchronous external abort or system crash.
- [High] Incorrect use of u32_replace_bits() combined with bitwise OR (|=) fails to clear existing bits.
- [High] Missing Root Complex mode configuration for the K3 controller due to a copy-paste error.
- [Medium] A bitmask macro for a completely different field is incorrectly passed as the replacement value to u32_replace_bits(), writing unexpected values.
- [Medium] Asymmetric teardown: k1_pcie_deinit() fails to assert reset on the K3 controller because it uses a reset line the controller is explicitly configured to ignore. --
commit 2826ac7fe9e0d0c0dd248a7fc412ce12f4d7b491
Author: Inochi Amaoto <[email protected]>
PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
This commit adds host controller support for the Spacemit K3 PCIe IP. It introduces K3-specific initialization, PHY enablement, and MSI host setup, hooking these into the existing spacemit-k1 driver.
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0
index 7f6f1df31cd81..7854d26220a98 100644
[ ... ]
Quoted 1 line
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0 -0
@@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {
[ ... ]
Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+static int k3_pcie_init(struct dw_pcie_rp *pp)
+{
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 338
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +6 -0
+	/*
+	 * Put the controller in root complex mode, and indicate that
+	 * Vaux (3.3v) is present.
+	 */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
Does this code configure the controller as a Root Complex?

Quoted 4 lines

The comment mentions putting the controller in root complex mode, but the register write only sets the PERSTN bits. In the K1 initialization, this is done by setting DEVICE_TYPE_RC, which appears to be missing here.

Unlike K1, K3 does not needs this.

Quoted 11 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 342
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4 -0
+
+	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
+	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
+			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
Is the replacement value intended to be GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE?
This passes a bitmask macro for a completely different field into u32_replace_bits(), which shifts the value into the target mask and writes an unexpected value into the register.

The GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE should be BIT(7), it is a mistake.

Quoted 15 lines

Quoted 1 line
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 365
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0
+static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
+	val |= u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
+				K3_CACHE_MSTR_AWCACHE_MODE);
Will this clear the existing bits correctly?

Quoted 4 lines

The u32_replace_bits() macro returns the fully modified 32-bit word. By using the bitwise OR assignment (|=), the original bits from val are ORed with the new bits, leaving any originally set bits as 1 instead of clearing them.

Yes, it is wrong, I will fix it.

Quoted 31 lines

Quoted 1 line
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 375
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
+static const struct dw_pcie_host_ops k3_pcie_host_ops = {
+	.init		= k3_pcie_init,
+	.deinit		= k1_pcie_deinit,
Can k1_pcie_deinit() successfully assert the reset for the K3 controller?
During initialization, k3_pcie_init() sets PCIE_IGNORE_PERSTN in PCIE_CONTROL_LOGIC. If k1_pcie_deinit() attempts to assert reset using PCIE_RC_PERST, the hardware might ignore it, leaving the device improperly reset during teardown.
Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+	.msi_init	= k3_pcie_msi_host_init,
+};
[ ... ]
Quoted 14 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +14 -0
+static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
+				     u32 *status0, u32 *status1, u32 *status2)
+{
+	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	*status1 = readl_relaxed(k1->link + INTR_STATUS);
+	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
+
+	writel_relaxed(*status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	writel_relaxed(*status1, k1->link + INTR_STATUS);
+	writel_relaxed(*status2, k1->link + K3_ADDR_INTR_STATUS1);
+}
+
+static int k3_pcie_parse_port(struct k1_pcie *k1)
+{
[ ... ]

Quoted 12 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 425
Quoted 8 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +8 -0
+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
+
+	return 0;
+}
Could accessing these MMIO registers before the clocks are enabled cause a synchronous external abort?

No problems, these register is always ready.

Quoted 10 lines

The probe function calls parse_port() before dw_pcie_host_init(). This means k3_pcie_clear_irq_status() performs direct MMIO reads and writes to the k1->link registers before k1_pcie_enable_resources() is called later in k3_pcie_init().
Accessing an unclocked peripheral can cause a bus stall on ARM/RISC-V architectures.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

Manivannan Sadhasivam

maintainer

2026-06-09 14:18 UTC #18 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

In reply to Inochi Amaoto

Quoted 18 lines · Inochi Amaoto

The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.
Add driver to support PCIe controller on Spacemit K3 PCIe.
Signed-off-by Inochi Amaoto <[email protected]>
---
 drivers/pci/controller/dwc/Kconfig            |   4 +-
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 169 ++++++++++++++++++
 2 files changed, 171 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/Kconfig +0 -0
index f2fde13107f2..fae971ecd876 100644

drivers/pci/controller/dwc/Kconfig config PCIE_SOPHGO_DW · line 442

Quoted 5 lines

drivers/pci/controller/dwc/Kconfig +1 -1

@@ -439,7 +439,7 @@ config PCIE_SOPHGO_DW
 	  Sophgo SoCs.
 
 config PCIE_SPACEMIT_K1
-	tristate "SpacemiT K1 PCIe controller (host mode)"
+	tristate "SpacemiT K1/K3 PCIe controller (host mode)"

Can you just say 'SpacemiT PCIe controller (host mode)"? I believe I asked Alex while adding K1 support and he said this driver might not support future IP revisions, but here we are.

Quoted 32 lines

drivers/pci/controller/dwc/Kconfig +1 -1

 	depends on ARCH_SPACEMIT || COMPILE_TEST
 	depends on HAS_IOMEM
 	select PCIE_DW_HOST

@@ -447,7 +447,7 @@ config PCIE_SPACEMIT_K1
 	default ARCH_SPACEMIT
 	help
 	  Enables support for the DesignWare based PCIe controller in
-	  the SpacemiT K1 SoC operating in host mode.  Three controllers
+	  the SpacemiT K1/K3 SoC operating in host mode. Three controllers
 	  are available on the K1 SoC; the first of these shares a PHY
 	  with a USB 3.0 host controller (one or the other can be used).

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0

index 7f6f1df31cd8..7854d26220a9 100644
@@ -23,6 +23,7 @@
 
 #define PCI_VENDOR_ID_SPACEMIT		0x201f
 #define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
+#define PCI_DEVICE_ID_SPACEMIT_K3	0x0002
 
 /* Offsets and field definitions for link management registers */
 #define K1_PHY_AHB_IRQ_EN			0x0000

@@ -32,8 +33,20 @@
 #define SMLH_LINK_UP			BIT(1)
 #define RDLH_LINK_UP			BIT(12)
 
+#define INTR_STATUS				0x0010
+
 #define INTR_ENABLE				0x0014
 #define MSI_CTRL_INT			BIT(11)

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 49

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+#define RDLH_LINK_UP_INT		BIT(20)
+
+#define K3_PHY_AHB_IRQSTATUS_INTX		0x0008
+
+#define K3_ADDR_INTR_STATUS1			0x0018
+
+#define K3_CACHE_MSTR_AWCACHE_MODE	GENMASK(14, 11)
+#define K3_CACHE_MSTR_AWCACHE_BEHAVIOR	0xf
+
+#define K3_MAX_PHY_NUMBER		6

What does this mean? 6 ports?

Quoted 52 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +38 -0

 /* Some controls require APMU regmap access */
 #define SYSCON_APMU			"spacemit,apmu"

@@ -48,6 +61,9 @@
 
 #define PCIE_CONTROL_LOGIC			0x0004
 #define PCIE_SOFT_RESET			BIT(0)
+#define PCIE_PERSTN_OE			BIT(24)
+#define PCIE_PERSTN_OUT			BIT(25)
+#define PCIE_IGNORE_PERSTN		BIT(31)
 
 struct k1_pcie {
 	struct dw_pcie pci;

@@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {
 	.stop_link	= k1_pcie_stop_link,
 };
 
+static int k3_pcie_enable_phy(struct k1_pcie *pcie)
+{
+	int i, ret;
+
+	for (i = 0; i < pcie->phy_count; i++) {
+		ret = phy_init(pcie->phy[i]);
+		if (ret)
+			goto err_phy;
+	}
+
+	return 0;
+
+err_phy:
+	while (--i >= 0)
+		phy_exit(pcie->phy[i]);
+
+	return ret;
+}
+
+static int k3_pcie_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 reset_ctrl = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
+	u32 val;
+	int ret;
+
+	regmap_clear_bits(k1->pmu, reset_ctrl, LTSSM_EN);
+
+	k1_pcie_toggle_soft_reset(k1);
+
+	ret = k1_pcie_enable_resources(k1);
+	if (ret)
+		return ret;
+

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 325

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+	regmap_set_bits(k1->pmu, reset_ctrl, PCIE_AUX_PWR_DET);
+	regmap_clear_bits(k1->pmu, reset_ctrl, APP_HOLD_PHY_RST);
+
+	ret = k3_pcie_enable_phy(k1);
+	if (ret) {
+		k1_pcie_disable_resources(k1);
+		return ret;
+	}
+
+	/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */

What does this mean?

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 336

Quoted 11 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +11 -0

+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT);
+	usleep_range(1000, 2000);
+	regmap_clear_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC, PCIE_PERSTN_OUT);
+
+	msleep(PCIE_T_PVPERL_MS);
+
+	/*
+	 * Put the controller in root complex mode, and indicate that
+	 * Vaux (3.3v) is present.
+	 */

How can the driver confirm without checking DT for vpcie3v3aux-supply?

Quoted 79 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +79 -0

+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
+
+	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
+	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
+			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_SPACEMIT);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_SPACEMIT_K3);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/* Finally, as a workaround, disable ASPM L1 */
+	k1_pcie_disable_aspm_l1(k1);
+
+	return 0;
+}
+
+static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
+	val |= u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
+				K3_CACHE_MSTR_AWCACHE_MODE);
+	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops k3_pcie_host_ops = {
+	.init		= k3_pcie_init,
+	.deinit		= k1_pcie_deinit,
+	.msi_init	= k3_pcie_msi_host_init,
+};
+
+static const struct dw_pcie_ops k3_pcie_ops = {
+	.link_up	= k1_pcie_link_up,
+	.start_link	= k1_pcie_start_link,
+	.stop_link	= k1_pcie_stop_link,
+};
+
+static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
+				     u32 *status0, u32 *status1, u32 *status2)
+{
+	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	*status1 = readl_relaxed(k1->link + INTR_STATUS);
+	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
+
+	writel_relaxed(*status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	writel_relaxed(*status1, k1->link + INTR_STATUS);
+	writel_relaxed(*status2, k1->link + K3_ADDR_INTR_STATUS1);
+}
+
+static int k3_pcie_parse_port(struct k1_pcie *k1)
+{
+	struct device *dev = k1->pci.dev;
+	u32 status0, status1, status2;
+	int i;
+
+	k1->phy = devm_kmalloc_array(dev, K3_MAX_PHY_NUMBER, sizeof(*k1->phy),
+				     GFP_KERNEL);
+	if (!k1->phy)
+		return -ENOMEM;
+
+	for (i = 0; i < K3_MAX_PHY_NUMBER; i++) {
+		k1->phy[i] = devm_of_phy_get_by_index(dev, dev->of_node, i);
+		if (IS_ERR(k1->phy[i])) {
+			if (PTR_ERR(k1->phy[i]) == -ENODEV)
+				break;
+
+			return PTR_ERR(k1->phy[i]);
+		}

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 425

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+	}
+
+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
+
+	return 0;
+}

This function should iterate over the Root Port nodes defined in DT.

- Mani

Signature · 1 line

மணிவண்ணன் சதாசிவம்

Andy Shevchenko

2026-06-09 14:49 UTC #19 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

Quoted 7 lines · Manivannan Sadhasivam

Quoted 6 lines · Inochi Amaoto
The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.
Add driver to support PCIe controller on Spacemit K3 PCIe.

...

Quoted 3 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 283
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
+static int k3_pcie_enable_phy(struct k1_pcie *pcie)
+{
+	int i, ret;

No need to have i signed.

Quoted 11 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 294
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +11 -0
+
+	for (i = 0; i < pcie->phy_count; i++) {
+		ret = phy_init(pcie->phy[i]);
+		if (ret)
+			goto err_phy;
+	}
+
+	return 0;
+
+err_phy:
+	while (--i >= 0)

	while (i--)

is shorter form of the same.

Quoted 2 lines

Quoted 2 lines
(quoted diff) +2 -0
+		phy_exit(pcie->phy[i]);
+

...

Quoted 3 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 328
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT);
+	usleep_range(1000, 2000);

fsleep(1 * USEC_PER_MSEC)

Quoted 3 lines

Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
+	regmap_clear_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC, PCIE_PERSTN_OUT);
+
+	msleep(PCIE_T_PVPERL_MS);

...

Quoted 1 line

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 401
Quoted 1 line
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
+	int i;

Why is 'i' signed?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Alex Elder

2026-06-09 16:11 UTC #20 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

In reply to Manivannan Sadhasivam

Quoted 21 lines · Manivannan Sadhasivam

Quoted 20 lines · Inochi Amaoto
The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.
Add driver to support PCIe controller on Spacemit K3 PCIe.
Signed-off-by Inochi Amaoto <[email protected]>
---
  drivers/pci/controller/dwc/Kconfig            |   4 +-
  drivers/pci/controller/dwc/pcie-spacemit-k1.c | 169 ++++++++++++++++++
  2 files changed, 171 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/Kconfig +0 -0
index f2fde13107f2..fae971ecd876 100644
@@ -439,7 +439,7 @@ config PCIE_SOPHGO_DW
  	  Sophgo SoCs.
 

Quoted 7 lines

drivers/pci/controller/dwc/Kconfig config PCIE_SOPHGO_DW · line 442
Quoted 3 lines
drivers/pci/controller/dwc/Kconfig +1 -1
  config PCIE_SPACEMIT_K1
-	tristate "SpacemiT K1 PCIe controller (host mode)"
+	tristate "SpacemiT K1/K3 PCIe controller (host mode)"
Can you just say 'SpacemiT PCIe controller (host mode)"? I believe I asked Alex while adding K1 support and he said this driver might not support future IP revisions, but here we are.

(Sorry if I said that in error.) Will this be K1 and K3 only? If so what you say is fine, but I agree with Mani, if it's going to also form the basis of K5 (or whatever) just make it "SpacemiT". And probably change the Kconfig symbol to PCIE_SPACEMIT.

-Alex

Quoted 12 lines

drivers/pci/controller/dwc/Kconfig +1 -1

  	depends on ARCH_SPACEMIT || COMPILE_TEST
  	depends on HAS_IOMEM
  	select PCIE_DW_HOST

@@ -447,7 +447,7 @@ config PCIE_SPACEMIT_K1
  	default ARCH_SPACEMIT
  	help
  	  Enables support for the DesignWare based PCIe controller in
-	  the SpacemiT K1 SoC operating in host mode.  Three controllers
+	  the SpacemiT K1/K3 SoC operating in host mode. Three controllers
  	  are available on the K1 SoC; the first of these shares a PHY
  	  with a USB 3.0 host controller (one or the other can be used).
  . . .

Alex Elder

2026-06-09 16:11 UTC #21 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

In reply to Inochi Amaoto

Quoted 6 lines · Inochi Amaoto

The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.
Add driver to support PCIe controller on Spacemit K3 PCIe.

It seems like you're creating a lot of new code for K3. In some cases it's very similar to K1, and it's not clear why it needs to be different.

I'd much rather see a patch that prepares for K3 support by doing minor refactoring of the existing code to support K1 in a way that will make adding K3 support more natural. Then a patch to enable K3 should be simpler (and can focus on what is truly different).

Quoted 42 lines

Signed-off-by Inochi Amaoto <[email protected]>

---
  drivers/pci/controller/dwc/Kconfig            |   4 +-
  drivers/pci/controller/dwc/pcie-spacemit-k1.c | 169 ++++++++++++++++++
  2 files changed, 171 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/Kconfig +2 -2

index f2fde13107f2..fae971ecd876 100644
@@ -439,7 +439,7 @@ config PCIE_SOPHGO_DW
  	  Sophgo SoCs.
 
  config PCIE_SPACEMIT_K1
-	tristate "SpacemiT K1 PCIe controller (host mode)"
+	tristate "SpacemiT K1/K3 PCIe controller (host mode)"
  	depends on ARCH_SPACEMIT || COMPILE_TEST
  	depends on HAS_IOMEM
  	select PCIE_DW_HOST

@@ -447,7 +447,7 @@ config PCIE_SPACEMIT_K1
  	default ARCH_SPACEMIT
  	help
  	  Enables support for the DesignWare based PCIe controller in
-	  the SpacemiT K1 SoC operating in host mode.  Three controllers
+	  the SpacemiT K1/K3 SoC operating in host mode. Three controllers
  	  are available on the K1 SoC; the first of these shares a PHY
  	  with a USB 3.0 host controller (one or the other can be used).

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0

index 7f6f1df31cd8..7854d26220a9 100644
@@ -23,6 +23,7 @@
 
  #define PCI_VENDOR_ID_SPACEMIT		0x201f
  #define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
+#define PCI_DEVICE_ID_SPACEMIT_K3	0x0002
 
  /* Offsets and field definitions for link management registers */
  #define K1_PHY_AHB_IRQ_EN			0x0000

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 42

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +5 -0

@@ -32,8 +33,20 @@
  #define SMLH_LINK_UP			BIT(1)
  #define RDLH_LINK_UP			BIT(12)
 
+#define INTR_STATUS				0x0010
+
  #define INTR_ENABLE				0x0014
  #define MSI_CTRL_INT			BIT(11)
+#define RDLH_LINK_UP_INT		BIT(20)
+
+#define K3_PHY_AHB_IRQSTATUS_INTX		0x0008

Can you add INTX support for K1 as well (perhaps in a separate patch)?

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 49

Quoted 6 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +6 -0

+#define K3_ADDR_INTR_STATUS1			0x0018
+
+#define K3_CACHE_MSTR_AWCACHE_MODE	GENMASK(14, 11)
+#define K3_CACHE_MSTR_AWCACHE_BEHAVIOR	0xf
+
+#define K3_MAX_PHY_NUMBER		6

You used "count" in patch 2 as the field name.

Quoted 14 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0

  /* Some controls require APMU regmap access */
  #define SYSCON_APMU			"spacemit,apmu"

@@ -48,6 +61,9 @@
 
  #define PCIE_CONTROL_LOGIC			0x0004
  #define PCIE_SOFT_RESET			BIT(0)
+#define PCIE_PERSTN_OE			BIT(24)
+#define PCIE_PERSTN_OUT			BIT(25)
+#define PCIE_IGNORE_PERSTN		BIT(31)
 
  struct k1_pcie {
  	struct dw_pcie pci;

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 281

Quoted 4 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0

@@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {
  	.stop_link	= k1_pcie_stop_link,
  };
 
+static int k3_pcie_enable_phy(struct k1_pcie *pcie)

Can you just make K1's single PHY be a special case of having "N" PHYs? I.e., just set the phy_count for K1 to be 1, so this loop would work for both K1 and K3?

Quoted 17 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +17 -0

+{
+	int i, ret;
+
+	for (i = 0; i < pcie->phy_count; i++) {
+		ret = phy_init(pcie->phy[i]);
+		if (ret)
+			goto err_phy;
+	}
+
+	return 0;
+
+err_phy:
+	while (--i >= 0)
+		phy_exit(pcie->phy[i]);
+
+	return ret;
+}

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 308

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+
+static int k3_pcie_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 reset_ctrl = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
+	u32 val;
+	int ret;
+
+	regmap_clear_bits(k1->pmu, reset_ctrl, LTSSM_EN);

Should the above be done for K1? Would it hurt? Handle both K1 and K3 the same way if possible.

The next two things are identical to k1_pcie_init(). Make the code common if possible, so it's very obvious what really needs to be different between the two.

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 310

Quoted 2 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+
+	k1_pcie_toggle_soft_reset(k1);

The "k1" prefix is fine for now, but if this driver gets used for more devices in the future, it might be worth renaming things to emphasize that it's not K1-specific.

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 318

Quoted 7 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +7 -0

+	ret = k1_pcie_enable_resources(k1);
+	if (ret)
+		return ret;
+
+	regmap_set_bits(k1->pmu, reset_ctrl, PCIE_AUX_PWR_DET);
+	regmap_clear_bits(k1->pmu, reset_ctrl, APP_HOLD_PHY_RST);
+

You enable the PHY here much earlier than what's done in the K1 code. Should the K1 PHY be enabled earlier? Also, I don't really think there needs to be separate versions of the code that enables PHYs for K1 and K3.

Quoted 16 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +16 -0

+	ret = k3_pcie_enable_phy(k1);
+	if (ret) {
+		k1_pcie_disable_resources(k1);
+		return ret;
+	}
+
+	/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT);
+	usleep_range(1000, 2000);
+	regmap_clear_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC, PCIE_PERSTN_OUT);
+
+	msleep(PCIE_T_PVPERL_MS);
+
+	/*
+	 * Put the controller in root complex mode, and indicate that

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 344

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0

+	 * Vaux (3.3v) is present.
+	 */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
+
+	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
+	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
+			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
+

The following block of code (roughly) is done right after enabling resources in the K1 version of this function.

Maybe the order you do it is better, but in that case, change the (existing, and soon, common) code to do it however is best if that's the case.

You should try to factor out the common parts and minimize what's actually different between the two.

I would also expect that the device ID would be stored in the platform data rather than having both init functions hard-code the value here.

I'm going to leave it at that for now.

-Alex

Quoted 101 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +89 -0

+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_SPACEMIT);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_SPACEMIT_K3);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/* Finally, as a workaround, disable ASPM L1 */
+	k1_pcie_disable_aspm_l1(k1);
+
+	return 0;
+}
+
+static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
+	val |= u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
+				K3_CACHE_MSTR_AWCACHE_MODE);
+	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops k3_pcie_host_ops = {
+	.init		= k3_pcie_init,
+	.deinit		= k1_pcie_deinit,
+	.msi_init	= k3_pcie_msi_host_init,
+};
+
+static const struct dw_pcie_ops k3_pcie_ops = {
+	.link_up	= k1_pcie_link_up,
+	.start_link	= k1_pcie_start_link,
+	.stop_link	= k1_pcie_stop_link,
+};
+
+static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
+				     u32 *status0, u32 *status1, u32 *status2)
+{
+	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	*status1 = readl_relaxed(k1->link + INTR_STATUS);
+	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
+
+	writel_relaxed(*status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	writel_relaxed(*status1, k1->link + INTR_STATUS);
+	writel_relaxed(*status2, k1->link + K3_ADDR_INTR_STATUS1);
+}
+
+static int k3_pcie_parse_port(struct k1_pcie *k1)
+{
+	struct device *dev = k1->pci.dev;
+	u32 status0, status1, status2;
+	int i;
+
+	k1->phy = devm_kmalloc_array(dev, K3_MAX_PHY_NUMBER, sizeof(*k1->phy),
+				     GFP_KERNEL);
+	if (!k1->phy)
+		return -ENOMEM;
+
+	for (i = 0; i < K3_MAX_PHY_NUMBER; i++) {
+		k1->phy[i] = devm_of_phy_get_by_index(dev, dev->of_node, i);
+		if (IS_ERR(k1->phy[i])) {
+			if (PTR_ERR(k1->phy[i]) == -ENODEV)
+				break;
+
+			return PTR_ERR(k1->phy[i]);
+		}
+	}
+
+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
+
+	return 0;
+}
+
  static int k1_pcie_parse_port(struct k1_pcie *k1)
  {
  	struct device *dev = k1->pci.dev;

@@ -363,8 +525,15 @@ static const struct k1_pcie_device_data k1_pcie_device_data = {
  	.parse_port	= k1_pcie_parse_port,
  };
 
+static const struct k1_pcie_device_data k3_pcie_device_data = {
+	.host_ops	= &k3_pcie_host_ops,
+	.ops		= &k3_pcie_ops,
+	.parse_port	= k3_pcie_parse_port,
+};
+
  static const struct of_device_id k1_pcie_of_match_table[] = {
  	{ .compatible = "spacemit,k1-pcie", .data = &k1_pcie_device_data},
+	{ .compatible = "spacemit,k3-pcie", .data = &k3_pcie_device_data},
  	{ }
  };

Inochi Amaoto

author

2026-06-22 08:50 UTC #22 lore raw

Re: [PATCH v2 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

Quoted 12 lines · Alex Elder

Quoted 6 lines · Inochi Amaoto
The PCIe controller on Spacemit K3 is almost a standard Synopsys DesignWare PCIe IP with extra link and reset control. Unlike the PCIe controller on K1, this controller supports external MSI interrupt controller and can use multiple PHYs at the same time.
Add driver to support PCIe controller on Spacemit K3 PCIe.
It seems like you're creating a lot of new code for K3. In some cases it's very similar to K1, and it's not clear why it needs to be different.

Quoted 6 lines

I'd much rather see a patch that prepares for K3 support by doing minor refactoring of the existing code to support K1 in a way that will make adding K3 support more natural. Then a patch to enable K3 should be simpler (and can focus on what is truly different).

I can not understand what you see, the init logic of K3 is completely different from the K1, so a new function is need. For other logic, I have reused all things as I can.

Quoted 42 lines

Signed-off-by Inochi Amaoto <[email protected]>

---
  drivers/pci/controller/dwc/Kconfig            |   4 +-
  drivers/pci/controller/dwc/pcie-spacemit-k1.c | 169 ++++++++++++++++++
  2 files changed, 171 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/Kconfig +2 -2

index f2fde13107f2..fae971ecd876 100644
@@ -439,7 +439,7 @@ config PCIE_SOPHGO_DW
  	  Sophgo SoCs.
  config PCIE_SPACEMIT_K1
-	tristate "SpacemiT K1 PCIe controller (host mode)"
+	tristate "SpacemiT K1/K3 PCIe controller (host mode)"
  	depends on ARCH_SPACEMIT || COMPILE_TEST
  	depends on HAS_IOMEM
  	select PCIE_DW_HOST

@@ -447,7 +447,7 @@ config PCIE_SPACEMIT_K1
  	default ARCH_SPACEMIT
  	help
  	  Enables support for the DesignWare based PCIe controller in
-	  the SpacemiT K1 SoC operating in host mode.  Three controllers
+	  the SpacemiT K1/K3 SoC operating in host mode. Three controllers
  	  are available on the K1 SoC; the first of these shares a PHY
  	  with a USB 3.0 host controller (one or the other can be used).

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0

index 7f6f1df31cd8..7854d26220a9 100644
@@ -23,6 +23,7 @@
  #define PCI_VENDOR_ID_SPACEMIT		0x201f
  #define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
+#define PCI_DEVICE_ID_SPACEMIT_K3	0x0002
  /* Offsets and field definitions for link management registers */
  #define K1_PHY_AHB_IRQ_EN			0x0000

@@ -32,8 +33,20 @@
  #define SMLH_LINK_UP			BIT(1)
  #define RDLH_LINK_UP			BIT(12)
+#define INTR_STATUS				0x0010
+

Quoted 9 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 42
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
  #define INTR_ENABLE				0x0014
  #define MSI_CTRL_INT			BIT(11)
+#define RDLH_LINK_UP_INT		BIT(20)
+
+#define K3_PHY_AHB_IRQSTATUS_INTX		0x0008
Can you add INTX support for K1 as well (perhaps in a separate patch)?

I have no knowledge about INTx support, and the vendor refuse to provide any information about this. So no support for this.

Quoted 19 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c line 49

Quoted 6 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +6 -0

+#define K3_ADDR_INTR_STATUS1			0x0018
+
+#define K3_CACHE_MSTR_AWCACHE_MODE	GENMASK(14, 11)
+#define K3_CACHE_MSTR_AWCACHE_BEHAVIOR	0xf
+
+#define K3_MAX_PHY_NUMBER		6

You used "count" in patch 2 as the field name.

Quoted 10 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0

  /* Some controls require APMU regmap access */
  #define SYSCON_APMU			"spacemit,apmu"

@@ -48,6 +61,9 @@
  #define PCIE_CONTROL_LOGIC			0x0004
  #define PCIE_SOFT_RESET			BIT(0)
+#define PCIE_PERSTN_OE			BIT(24)
+#define PCIE_PERSTN_OUT			BIT(25)
+#define PCIE_IGNORE_PERSTN		BIT(31)
  struct k1_pcie {
  	struct dw_pcie pci;

Quoted 9 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 280
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1 -0
@@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {
  	.stop_link	= k1_pcie_stop_link,
  };
+static int k3_pcie_enable_phy(struct k1_pcie *pcie)
Can you just make K1's single PHY be a special case of having "N" PHYs? I.e., just set the phy_count for K1 to be 1, so this loop would work for both K1 and K3?

This is reasonable.

Quoted 31 lines

Quoted 17 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +17 -0
+{
+	int i, ret;
+
+	for (i = 0; i < pcie->phy_count; i++) {
+		ret = phy_init(pcie->phy[i]);
+		if (ret)
+			goto err_phy;
+	}
+
+	return 0;
+
+err_phy:
+	while (--i >= 0)
+		phy_exit(pcie->phy[i]);
+
+	return ret;
+}
drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 308
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0
+
+static int k3_pcie_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 reset_ctrl = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
+	u32 val;
+	int ret;
+
+	regmap_clear_bits(k1->pmu, reset_ctrl, LTSSM_EN);
Should the above be done for K1? Would it hurt? Handle both K1 and K3 the same way if possible.

Quoted 4 lines

The next two things are identical to k1_pcie_init(). Make the code common if possible, so it's very obvious what really needs to be different between the two.

No, I have diff the code and find only about 40% of logic are similar. However, these logic is fragmented, so I decide to add a new function, which is easier to maintain.

Quoted 7 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 309
Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2 -0
+
+	k1_pcie_toggle_soft_reset(k1);
The "k1" prefix is fine for now, but if this driver gets used for more devices in the future, it might be worth renaming things to emphasize that it's not K1-specific.

Currently I have no knowledge on the new device and do not think it is a good idea to change the name. Anyway, it is possible to change to kx, but I do not think it is clear.

Quoted 4 lines

Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4 -0
+	ret = k1_pcie_enable_resources(k1);
+	if (ret)
+		return ret;
+

Quoted 9 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 318
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3 -0
+	regmap_set_bits(k1->pmu, reset_ctrl, PCIE_AUX_PWR_DET);
+	regmap_clear_bits(k1->pmu, reset_ctrl, APP_HOLD_PHY_RST);
+
You enable the PHY here much earlier than what's done in the K1 code. Should the K1 PHY be enabled earlier? Also, I don't really think there needs to be separate versions of the code that enables PHYs for K1 and K3.

IIRC this is a specific logic for K3.

Quoted 37 lines

Quoted 16 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +16 -0
+	ret = k3_pcie_enable_phy(k1);
+	if (ret) {
+		k1_pcie_disable_resources(k1);
+		return ret;
+	}
+
+	/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT);
+	usleep_range(1000, 2000);
+	regmap_clear_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC, PCIE_PERSTN_OUT);
+
+	msleep(PCIE_T_PVPERL_MS);
+
+	/*
+	 * Put the controller in root complex mode, and indicate that
drivers/pci/controller/dwc/pcie-spacemit-k1.c static const struct dw_pcie_ops k1_pcie_ops = { · line 344
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10 -0
+	 * Vaux (3.3v) is present.
+	 */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
+			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
+
+	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
+	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
+			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
+
The following block of code (roughly) is done right after enabling resources in the K1 version of this function.
Maybe the order you do it is better, but in that case, change the (existing, and soon, common) code to do it however is best if that's the case.
You should try to factor out the common parts and minimize what's actually different between the two.

Quoted 7 lines

I would also expect that the device ID would be stored in the platform data rather than having both init functions hard-code the value here.
I'm going to leave it at that for now.
-Alex

This first thing you should know is the init logic of K3 is totally different from K1, I have asked for the vendor and they told me it is better to treat them differently, even if they are using the same ip.

For the request of the device ID setup, I think I can add a function for it.

Quoted 101 lines

Quoted 100 lines

drivers/pci/controller/dwc/pcie-spacemit-k1.c +89 -0

+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_SPACEMIT);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_SPACEMIT_K3);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/* Finally, as a workaround, disable ASPM L1 */
+	k1_pcie_disable_aspm_l1(k1);
+
+	return 0;
+}
+
+static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
+	val |= u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
+				K3_CACHE_MSTR_AWCACHE_MODE);
+	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops k3_pcie_host_ops = {
+	.init		= k3_pcie_init,
+	.deinit		= k1_pcie_deinit,
+	.msi_init	= k3_pcie_msi_host_init,
+};
+
+static const struct dw_pcie_ops k3_pcie_ops = {
+	.link_up	= k1_pcie_link_up,
+	.start_link	= k1_pcie_start_link,
+	.stop_link	= k1_pcie_stop_link,
+};
+
+static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
+				     u32 *status0, u32 *status1, u32 *status2)
+{
+	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	*status1 = readl_relaxed(k1->link + INTR_STATUS);
+	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
+
+	writel_relaxed(*status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
+	writel_relaxed(*status1, k1->link + INTR_STATUS);
+	writel_relaxed(*status2, k1->link + K3_ADDR_INTR_STATUS1);
+}
+
+static int k3_pcie_parse_port(struct k1_pcie *k1)
+{
+	struct device *dev = k1->pci.dev;
+	u32 status0, status1, status2;
+	int i;
+
+	k1->phy = devm_kmalloc_array(dev, K3_MAX_PHY_NUMBER, sizeof(*k1->phy),
+				     GFP_KERNEL);
+	if (!k1->phy)
+		return -ENOMEM;
+
+	for (i = 0; i < K3_MAX_PHY_NUMBER; i++) {
+		k1->phy[i] = devm_of_phy_get_by_index(dev, dev->of_node, i);
+		if (IS_ERR(k1->phy[i])) {
+			if (PTR_ERR(k1->phy[i]) == -ENODEV)
+				break;
+
+			return PTR_ERR(k1->phy[i]);
+		}
+	}
+
+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
+
+	return 0;
+}
+
  static int k1_pcie_parse_port(struct k1_pcie *k1)
  {
  	struct device *dev = k1->pci.dev;

@@ -363,8 +525,15 @@ static const struct k1_pcie_device_data k1_pcie_device_data = {
  	.parse_port	= k1_pcie_parse_port,
  };
+static const struct k1_pcie_device_data k3_pcie_device_data = {
+	.host_ops	= &k3_pcie_host_ops,
+	.ops		= &k3_pcie_ops,
+	.parse_port	= k3_pcie_parse_port,
+};
+
  static const struct of_device_id k1_pcie_of_match_table[] = {
  	{ .compatible = "spacemit,k1-pcie", .data = &k1_pcie_device_data},
+	{ .compatible = "spacemit,k3-pcie", .data = &k3_pcie_device_data},
  	{ }
  };