riscv: spacemit: Add PCIe RC controller support for K3
Subsystems (6)
v3 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 v2:
- https://lore.kernel.org/linux-pci/[email protected]/
patch 2:
1. Use unify PCIe phy get/enable/exit function for both K1 and K3. patch 3:
1. New patch for updating device id for both K1/K3
patch 5:
1. Reuse binding for spacemit,k1-pcie-host
patch 6:
1. Multiple cleanup for the logic already generalized in the previous patches.
2. Use generic description for the Kconfig entry.
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 (6):
PCI: spacemit-k1: Add device data support
PCI: spacemit-k1: Add multiple PHY handles support
PCI: spacemit-k1: Add device id update helper
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,k1-pcie-host.yaml | 46 +++- drivers/pci/controller/dwc/Kconfig | 4 +- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 237 ++++++++++++++++-- 4 files changed, 267 insertions(+), 27 deletions(-)
--
2.55.0
PATCH v3 1/6 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.
--- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-)
drivers/pci/controller/dwc/pcie-spacemit-k1.c +26 -4
@@ -49,8 +49,17 @@#define PCIE_CONTROL_LOGIC 0x0004#define PCIE_SOFT_RESET BIT(0) +struct k1_pcie ; + +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); +}; +struct k1_pcie {struct dw_pcie pci; +const struct k1_pcie_device_data *data;struct phy *phy;void __iomem *link;struct regmap *pmu;/* Errors ignored; MMIO-backed regmap */
@@ -278,14 +287,21 @@ 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 ; + k1->data = data; + k1->pmu =syscon_regmap_lookup_by_phandle_args (dev_of_node (dev),SYSCON_APMU ,1 , &k1->pmu_off);
@@ -299,11 +315,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 +336,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 +354,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.55.0
[email protected]
AI reviewRe: [PATCH v3 1/6] PCI: spacemit-k1: Add device data support
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
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]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v3 2/6 PCI: spacemit-k1: Add multiple PHY handles support
In reply to Inochi AmaotoThe 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 | 70 ++++++++++++++++--- 1 file changed, 59 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-spacemit-k1.c +59 -11
@@ -55,12 +55,14 @@ 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); +unsigned int max_phy_count; };struct k1_pcie {struct dw_pcie pci;const struct k1_pcie_device_data *data; -struct phy *phy; +struct phy **phy; +unsigned int phy_count;void __iomem *link;struct regmap *pmu;/* Errors ignored; MMIO-backed regmap */ u32 pmu_off;
@@ -119,6 +121,54 @@ static void k1_pcie_disable_resources(struct k1_pcie *k1)clk_bulk_disable_unprepare (ARRAY_SIZE (pci->app_clks), pci->app_clks); } +static int k1_pcie_get_phy_handle (struct k1_pcie *k1,struct device_node *node) +{ +const struct k1_pcie_device_data *data = k1->data; +struct device *dev = k1->pci.dev; +unsigned int i; + + k1->phy =devm_kmalloc_array (dev, data->max_phy_count, +sizeof (*k1->phy),GFP_KERNEL ); +if (!k1->phy) +return -ENOMEM ; + +for (i =0 ; i < data->max_phy_count; i++) { + k1->phy[i] =devm_of_phy_get_by_index (dev, 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 ; + +return 0 ; +} + +static int k1_pcie_enable_phy (struct k1_pcie *k1) +{ +unsigned int i; +int ret; + +for (i =0 ; i < k1->phy_count; i++) { + ret =phy_init (k1->phy[i]); +if (ret) + goto err_phy; + } + +return 0 ; + +err_phy: +while (i--) +phy_exit (k1->phy[i]); + +return ret; +} +/* FIXME: Disable ASPM L1 to avoid errors reported on some NVMe drives */ static void k1_pcie_disable_aspm_l1 (struct k1_pcie *k1) {
@@ -174,7 +224,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 =k1_pcie_enable_phy (k1);if (ret) {k1_pcie_disable_resources (k1);
@@ -194,12 +244,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); }
@@ -266,23 +318,18 @@ static int k1_pcie_parse_port(struct k1_pcie *k1) {struct device *dev = k1->pci.dev;struct device_node *root_port; -struct phy *phy; +int ret;/* We assume only one root port */ root_port =of_get_next_available_child (dev_of_node (dev),NULL );if (!root_port)return -EINVAL ; - phy =devm_of_phy_get (dev, root_port,NULL ); + ret =k1_pcie_get_phy_handle (k1, root_port);of_node_put (root_port); -if (IS_ERR (phy)) -return PTR_ERR (phy); - - k1->phy = phy; - -return 0 ; +return ret; }static int k1_pcie_probe (struct platform_device *pdev)
@@ -358,6 +405,7 @@ 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, + .max_phy_count = 1, };static const struct of_device_id k1_pcie_of_match_table[] = {
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 2/6] PCI: spacemit-k1: Add multiple PHY handles support
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
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]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v3 3/6 PCI: spacemit-k1: Add device id update helper
In reply to Inochi AmaotoBoth K1 and K3 needs to set vendor id and device id, add a helper function to simplify this.
--- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/pci/controller/dwc/pcie-spacemit-k1.c +13 -4
@@ -56,6 +56,7 @@ struct k1_pcie_device_data {const struct dw_pcie_ops *ops;int (*parse_port)(struct k1_pcie *k1);unsigned int max_phy_count; +unsigned int device_id; };struct k1_pcie {
@@ -186,6 +187,16 @@ static void k1_pcie_disable_aspm_l1(struct k1_pcie *k1)dw_pcie_dbi_ro_wr_dis (pci); } +static void k1_pcie_set_device_id (struct k1_pcie *k1) +{ +struct dw_pcie *pci = &k1->pci; + +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 , k1->data->device_id); +dw_pcie_dbi_ro_wr_dis (pci); +} +static int k1_pcie_init (struct dw_pcie_rp *pp) {struct dw_pcie *pci =to_dw_pcie_from_pp (pp);
@@ -201,10 +212,7 @@ static int k1_pcie_init(struct dw_pcie_rp *pp)return ret;/* Set the PCI vendor and device ID */ -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_K1 ); -dw_pcie_dbi_ro_wr_dis (pci); +k1_pcie_set_device_id (k1);/* * Start by asserting fundamental reset (drive PERST# low). The
@@ -406,6 +414,7 @@ static const struct k1_pcie_device_data k1_pcie_device_data = { .ops = &k1_pcie_ops, .parse_port = k1_pcie_parse_port, .max_phy_count = 1, + .device_id = PCI_DEVICE_ID_SPACEMIT_K1, };static const struct of_device_id k1_pcie_of_match_table[] = {
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 3/6] PCI: spacemit-k1: Add device id update helper
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines
Both K1 and K3 needs to set vendor id and device id, add a helper function to simplify this.
Signed-off-by Inochi Amaoto <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v3 4/6 dt-bindings: PCI: snps,dw-pcie: Add msi-parent for MSI handle check
In reply to Inochi AmaotoThe 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.
--- 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
@@ -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.55.0
[email protected]
AI reviewRe: [PATCH v3 4/6] dt-bindings: PCI: snps,dw-pcie: Add msi-parent for MSI handle check
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines
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]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v3 5/6 dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
In reply to Inochi AmaotoAdd 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.
--- .../bindings/pci/spacemit,k1-pcie-host.yaml | 46 ++++++++++++++++--- 1 file changed, 39 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +39 -7
@@ -14,26 +14,29 @@ description: > PCIe IP. The controller uses the DesignWare built-in MSI interrupt controller, and supports 256 MSIs. -allOf: - - $ref: /schemas/pci/snps,dw-pcie.yaml# - properties: compatible: - const: spacemit,k1-pcie + enum: + - spacemit,k1-pcie + - spacemit,k3-pcie reg: + minItems: 4 items: - description: DesignWare PCIe registers - description: ATU address space - description: PCIe configuration space - description: Link control registers + - description: Data Bus Interface (DBI) shadow registers. reg-names: + minItems: 4 items: - const: dbi - const: atu - const: config - const: link + - const: dbi2 clocks: items:
@@ -66,6 +69,8 @@ properties: interrupt-names: const: msi + msi-parent: true + spacemit,apmu: $ref: /schemas/types.yaml#/definitions/phandle-array description:
@@ -84,7 +89,8 @@ patternProperties: properties: phys: - maxItems: 1 + minItems: 1 + maxItems: 6 vpcie3v3-supply: description:
@@ -96,13 +102,39 @@ patternProperties: unevaluatedProperties: false +allOf: + - $ref: /schemas/pci/snps,dw-pcie.yaml# + - if: + properties: + compatible: + contains: + const: spacemit,k1-pcie + then: + properties: + reg: + maxItems: 4 + + reg-names: + maxItems: 4 + + patternProperties: + '^pcie@': + properties: + phys: + maxItems: 1 + else: + properties: + reg: + minItems: 5 + + reg-names: + minItems: 5 + required: - clocks - clock-names - resets - reset-names - - interrupts - - interrupt-names - spacemit,apmu unevaluatedProperties: false
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 5/6] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `interrupts` and `interrupt-names` properties were incorrectly removed from the schema…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The interrupts and interrupt-names properties were incorrectly removed from the schema validation requirements for the older spacemit,k1-pcie variant. --
commit 5291e523faf87db00be6f9789a94fd879ec4c554
Author: Inochi Amaoto <[email protected]>
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
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.
Quoted 4 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-0
[ ... ]
Quoted 29 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +25
-0@@ -96,13 +102,39 @@ patternProperties: unevaluatedProperties: false +allOf: + - $ref: /schemas/pci/snps,dw-pcie.yaml# + - if: + properties: + compatible: + contains: + const: spacemit,k1-pcie + then: + properties: + reg: + maxItems: 4 + + reg-names: + maxItems: 4 + + patternProperties: + '^pcie@': + properties: + phys: + maxItems: 1 + else: + properties: + reg: + minItems: 5 +
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml patternProperties: · line 105
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +3
-2+ reg-names: + minItems: 5 + required: - clocks - clock-names - resets - reset-names - - interrupts - - interrupt-names
[Severity: Medium]
Will this removal cause incomplete device trees for spacemit,k1-pcie to silently pass schema validation if they omit the interrupts and interrupt-names properties?
Should these properties be added to a required list in the if-then block for spacemit,k1-pcie to preserve the previous constraint?
Quoted 3 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-0- spacemit,apmu unevaluatedProperties: false
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Re: [PATCH v3 5/6] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
In reply to Inochi AmaotoDocumentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml patternProperties: · line 105
Quoted 7 lines · Inochi Amaoto
required:
- clocks
- clock-names
- resets
- reset-namesDocumentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-2- - interrupts - - interrupt-names
Why?
Quoted 2 lines
- spacemit,apmu
Best regards,
Krzysztof
Re: [PATCH v3 5/6] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
Quoted 11 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml patternProperties: · line 105
Quoted 7 lines · Inochi Amaoto
required:
- clocks
- clock-names
- resets
- reset-namesDocumentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-2- - interrupts - - interrupt-namesWhy?
The K3 uses external MSI controller so it has no interrupt/interrupt-name. But I do make a mistake here as I forgot to restore the required for the K1 in the allOf check. This will be fixed in the next version.
Regards,
Inochi
Quoted 6 lines
Quoted 2 lines
- spacemit,apmu
Best regards,
Krzysztof
PATCH v3 6/6 PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
In reply to Inochi AmaotoThe 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.
--- drivers/pci/controller/dwc/Kconfig | 4 +- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 126 ++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/Kconfig +2 -2
@@ -440,7 +440,7 @@ config PCIE_SOPHGO_DW Sophgo SoCs. config PCIE_SPACEMIT_K1 - tristate "SpacemiT K1 PCIe controller (host mode)" + tristate "SpacemiT PCIe controller (host mode)" depends on ARCH_SPACEMIT || COMPILE_TEST depends on HAS_IOMEM select PCIE_DW_HOST
@@ -448,7 +448,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 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 +126 -0
160 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Unclocked MMIO access during probe triggers a synchronous bus abort. - [High] Incorrect `.deinit`…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unclocked MMIO access during probe triggers a synchronous bus abort.
- [High] Incorrect .deinit callback for K3 prevents PERST# assertion on teardown.
- [High] AXI cache coherency initialization is bypassed when MSIs are disabled.
- [Medium] Misleading comment and potential missing initialization for Root Complex mode. --
commit 93fac4ee5b6c6ef1d779cae9b2ff5efed7a8fb39
Author: Inochi Amaoto <[email protected]>
PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
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 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0
[ ... ]
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2
-0@@ -340,6 +354,109 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)return ret; } +static int k3_pcie_init (struct dw_pcie_rp *pp) +{
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 395
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 );
[Severity: Medium]
Will this omit initializing the controller into root complex mode?
Unlike k1_pcie_init(), this code block only drives the PERST# reset pins and seems to skip setting DEVICE_TYPE_RC.
Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2
-0+ + val =dw_pcie_readl_dbi (pci,GEN3_EQ_CONTROL_OFF );
[ ... ]
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +6
-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);
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 425
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ + 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 ; +}
[Severity: High]
Does this code miss initializing AXI cache coherency when MSIs are disabled?
Since k3_pcie_msi_host_init() is only called if pci_msi_enabled() is true, booting with pci=nomsi would skip this callback, silently skipping general cache coherency initialization for the controller.
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 429
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ +static const struct dw_pcie_host_ops k3_pcie_host_ops = { + .init = k3_pcie_init, + .deinit = k1_pcie_deinit,
[Severity: High]
Can reusing k1_pcie_deinit() prevent PERST# assertion on teardown?
During initialization, k3_pcie_init() bypasses the K1 mechanism by setting PCIE_IGNORE_PERSTN in PCIE_CONTROL_LOGIC. Since k1_pcie_deinit() attempts to toggle PCIE_RC_PERST, won't the K3 hardware ignore it?
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+ .msi_init = k3_pcie_msi_host_init, +}; + +static const struct dw_pcie_ops k3_pcie_ops = { + .link_up = k1_pcie_link_up,
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 444
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ .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 );
[Severity: High]
Could this read unclocked MMIO memory and trigger a synchronous bus abort?
In k1_pcie_probe(), data->parse_port() is called before dw_pcie_host_init():
k1_pcie_probe() {
...
ret = data->parse_port(k1);
...
ret = dw_pcie_host_init(&k1->pci.pp);
}This means k3_pcie_parse_port() and k3_pcie_clear_irq_status() are executed before the required hardware clocks are enabled, accessing hardware registers while their backing clocks are gated.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Re: [PATCH v3 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
Quoted 31 lines · On Fri, Jul 03, 2026 at 02:20:57AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unclocked MMIO access during probe triggers a synchronous bus abort.
- [High] Incorrect.deinitcallback for K3 prevents PERST# assertion on teardown.
- [High] AXI cache coherency initialization is bypassed when MSIs are disabled.
- [Medium] Misleading comment and potential missing initialization for Root Complex mode. --commit
93fac4ee5b6c6ef1d779cae9b2ff5efed7a8fb39
Author: Inochi Amaoto <[email protected]>PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
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 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0[ ... ]
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2
-0@@ -340,6 +354,109 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)return ret; } +static int k3_pcie_init (struct dw_pcie_rp *pp) +{[ ... ]
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 395
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 );[Severity: Medium]
Will this omit initializing the controller into root complex mode?
Yes
Quoted 18 lines
Unlike
k1_pcie_init(), this code block only drives the PERST# reset pins and seems to skip settingDEVICE_TYPE_RC.Quoted 2 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2
-0+ + val =dw_pcie_readl_dbi (pci,GEN3_EQ_CONTROL_OFF );[ ... ]
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +11
-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);
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 425
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+ +dw_pcie_dbi_ro_wr_dis (pci); + +return 0 ; +}[Severity: High]
Does this code miss initializing AXI cache coherency when MSIs are disabled?
No
Quoted 4 lines
Since
k3_pcie_msi_host_init()is only called ifpci_msi_enabled()is true, booting with pci=nomsi would skip this callback, silently skipping general cache coherency initialization for the controller.
The PCIe controller is broken as it does not support INTx in this driver.
Quoted 8 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 429
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ +static const struct dw_pcie_host_ops k3_pcie_host_ops = { + .init = k3_pcie_init, + .deinit = k1_pcie_deinit,[Severity: High]
Can reusingk1_pcie_deinit()prevent PERST# assertion on teardown?
Quoted 4 lines
During initialization,
k3_pcie_init()bypasses the K1 mechanism by settingPCIE_IGNORE_PERSTNinPCIE_CONTROL_LOGIC. Sincek1_pcie_deinit()attempts to togglePCIE_RC_PERST, won't the K3 hardware ignore it?
The K3 has handled this properly. You should treat the K3 and K1
totally differently.
Quoted 10 lines
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ .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,
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 444
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+ 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 );[Severity: High]
Could this read unclocked MMIO memory and trigger a synchronous bus abort?
No, it is possible to access.
Quoted 15 lines
In
k1_pcie_probe(),data->parse_port()is called beforedw_pcie_host_init():k1_pcie_probe() { ... ret = data->parse_port(k1); ... ret = dw_pcie_host_init(&k1->pci.pp); }This means
k3_pcie_parse_port()andk3_pcie_clear_irq_status()are executed before the required hardware clocks are enabled, accessing hardware registers while their backing clocks are gated.Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Quoted 48 lines · Inochi Amaoto
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 v2:
- https://lore.kernel.org/linux-pci/[email protected]/
patch 2:
1. Use unify PCIe phy get/enable/exit function for both K1 and K3. patch 3:
1. New patch for updating device id for both K1/K3
patch 5:
1. Reuse binding for spacemit,k1-pcie-host
patch 6:
1. Multiple cleanup for the logic already generalized in the previous patches.
2. Use generic description for theKconfigentry.Changed from v1:
patch 1:
1. Removepost_initcallback.
patch 2:
1. Fixdevm_kmalloc_arrayarguments 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. Fixdevm_kmalloc_arrayarguments order and error check.
4. Remove unnecessarystart_linklogic.
5. Make all magic number as a macro.
6. Fix spelling mistakes in code and commit message.Inochi Amaoto (6):
PCI: spacemit-k1: Add device data support
PCI: spacemit-k1: Add multiple PHY handles support
PCI: spacemit-k1: Add device id update helper
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
Quoted 6 lines
.../devicetree/bindings/pci/snps,dw-pcie.yaml | 7 +- .../bindings/pci/spacemit,k1-pcie-host.yaml | 46 +++- drivers/pci/controller/dwc/Kconfig | 4 +- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 237 ++++++++++++++++-- 4 files changed, 267 insertions(+), 27 deletions(-)
For patches 1, 2, 3 and 6:
Signature · 2 lines
Aurelien Jarno GPG: 4096R/1DDD8C9B [email protected] http://aurel32.net