riscv: spacemit: Add PCIe RC controller support for K3
Subsystems (6)
v5 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.
Since v5, multiple vendor fix are included in this series, you can find in [1]. And this version required [2] for phy bulk data support.
[1] https://github.com/spacemit-com/linux-6.18/commits/k3-br-v1.0.y/drivers/pci/controller/dwc/pcie-spacemit-k1.c
[2] https://lore.kernel.org/linux-phy/[email protected]/
Changed from v4:
- https://lore.kernel.org/linux-pci/[email protected]/
patch 1:
1. fix comments
patch 2:
1. Use phy bulk data to simplify the code
patch 3:
1. Rebased to latest master
patch 6:
1. Apply several fix from the vendor code
2. Use separated init/deinit function for K3.
3. Add some error handle for K3 init function
Changed from v3:
- https://lore.kernel.org/linux-pci/[email protected]/
patch 5:
1. Add missing interrupt/interrupt-names check for K1
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 | 50 ++++- drivers/pci/controller/dwc/Kconfig | 4 +- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 210 ++++++++++++++++-- 4 files changed, 244 insertions(+), 27 deletions(-)
--
2.55.0
PATCH v5 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
@@ -45,8 +45,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 */
@@ -274,14 +283,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);
@@ -295,11 +311,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,
@@ -316,7 +332,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" );
@@ -334,8 +350,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 }, { } };MODULE_DEVICE_TABLE (of, k1_pcie_of_match_table);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v5 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
Re: [PATCH v5 1/6] PCI: spacemit-k1: Add device data support
In reply to Inochi AmaotoQuoted 3 lines · Inochi Amaoto
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 static int k1_pcie_parse_port(struct k1_pcie *k1) · line 293
Quoted 10 lines
static int
k1_pcie_probe(struct platform_device *pdev){drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+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 ;
It's better to use -ENODATA which will help to distinguish from other ENODEV cases (which are more often to happen).
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0k1 =devm_kzalloc (dev,sizeof (*k1),GFP_KERNEL );if (!k1)return -ENOMEM ;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/6] PCI: spacemit-k1: Add device data support
Quoted 14 lines · Andy Shevchenko
Quoted 3 lines · Inochi Amaoto
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.
...
Quoted 7 lines
static int
k1_pcie_probe(struct platform_device *pdev){drivers/pci/controller/dwc/pcie-spacemit-k1.c +1
-0+const struct k1_pcie_device_data *data;struct device *dev = &pdev->dev;struct k1_pcie *k1;int ret;
Quoted 7 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 293
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3
-0+ data =device_get_match_data (dev); +if (!data) +return -ENODEV ;It's better to use
-ENODATAwhich will help to distinguish from other ENODEV cases (which are more often to happen).
OK, thanks
Quoted 9 lines
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0k1 =devm_kzalloc (dev,sizeof (*k1),GFP_KERNEL );if (!k1)return -ENOMEM ;Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/6] PCI: spacemit-k1: Add device data support
Quoted 23 lines · Inochi Amaoto
Quoted 14 lines · Andy Shevchenko
Quoted 3 lines · Inochi Amaoto
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.
...
Quoted 7 lines
static int
k1_pcie_probe(struct platform_device *pdev){drivers/pci/controller/dwc/pcie-spacemit-k1.c +1
-0+const struct k1_pcie_device_data *data;struct device *dev = &pdev->dev;struct k1_pcie *k1;int ret;Quoted 7 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 293
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3
-0+ data =device_get_match_data (dev); +if (!data) +return -ENODEV ;It's better to use
-ENODATAwhich will help to distinguish from other ENODEV cases (which are more often to happen).
Quoted 1 line
OK, thanks
Honestly, I don't think this should even happen, since every compatible supported by the driver carries its own of_match_data, and failing to do so is obviously a driver bug, so I think this branch could be dropped.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0k1 =devm_kzalloc (dev,sizeof (*k1),GFP_KERNEL );if (!k1)return -ENOMEM ;Signature · 2 lines
With Best Regards, Andy Shevchenko
Best regards,
Yao Zi
Re: [PATCH v5 1/6] PCI: spacemit-k1: Add device data support
Quoted 3 lines · Yao Zi
Quoted 2 lines · Inochi Amaoto
Quoted 1 line · Andy Shevchenko
On Mon, Sep 07, 2026 at 07:26:00PM +0800, Inochi Amaoto wrote:
...
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 293
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3
-0+ data =device_get_match_data (dev); +if (!data) +return -ENODEV ;It's better to use
-ENODATAwhich will help to distinguish from other ENODEV cases (which are more often to happen).
Quoted 6 lines
Quoted 1 line
OK, thanks
Honestly, I don't think this should even happen, since every compatible supported by the driver carries its own
of_match_data, and failing to do so is obviously a driver bug, so I think this branch could be dropped.
Does this driver do not support driver_override? How comes?
(Note, Sashiko is full of the issues of not checking for NULL the matching data
and hence it describes a scenario in which this leads to NULL pointer dereference.)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/6] PCI: spacemit-k1: Add device data support
Quoted 14 lines · Andy Shevchenko
Quoted 3 lines · Yao Zi
Quoted 2 lines · Inochi Amaoto
Quoted 1 line · Andy Shevchenko
On Mon, Sep 07, 2026 at 07:26:00PM +0800, Inochi Amaoto wrote:
...
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 293
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3
-0+ data =device_get_match_data (dev); +if (!data) +return -ENODEV ;It's better to use
-ENODATAwhich will help to distinguish from other ENODEV cases (which are more often to happen).
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
OK, thanks
Honestly, I don't think this should even happen, since every compatible supported by the driver carries its own
of_match_data, and failing to do so is obviously a driver bug, so I think this branch could be dropped.Does this driver do not support
driver_override? How comes?
Oops, it's my fault not to come up with this case. Sorry for the noise.
Quoted 9 lines
(Note, Sashiko is full of the issues of not checking for NULL the matching data
and hence it describes a scenario in which this leads to NULL pointer dereference.)Signature · 2 lines
With Best Regards, Andy Shevchenko
Thanks,
Yao Zi
PATCH v5 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 supported by the current driver. So extend the PHY definition to support multiple PHY handles.
--- drivers/pci/controller/dwc/pcie-spacemit-k1.c | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-spacemit-k1.c +26 -11
@@ -51,12 +51,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_bulk_data *phys; +unsigned int phy_count;void __iomem *link;struct regmap *pmu;/* Errors ignored; MMIO-backed regmap */ u32 pmu_off;
@@ -115,6 +117,23 @@ 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; +int count; + + count =devm_of_phy_bulk_get_all (dev, node, &k1->phys); +if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ; + + k1->phy_count = count; + +return 0 ; +} +/* FIXME: Disable ASPM L1 to avoid errors reported on some NVMe drives */ static void k1_pcie_disable_aspm_l1 (struct k1_pcie *k1) {
@@ -170,7 +189,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_bulk_init (k1->phy_count, k1->phys);if (ret) {k1_pcie_disable_resources (k1);
@@ -195,7 +214,7 @@ static void k1_pcie_deinit(struct dw_pcie_rp *pp)regmap_set_bits (k1->pmu, k1->pmu_off +PCIE_CLK_RESET_CONTROL ,PCIE_RC_PERST ); -phy_exit (k1->phy); +phy_bulk_exit (k1->phy_count, k1->phys);k1_pcie_disable_resources (k1); }
@@ -262,23 +281,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)
@@ -354,6 +368,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 v5 2/6] PCI: spacemit-k1: Add multiple PHY handles support
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The patch uses non-existent PHY bulk APIs (`struct phy_bulk_data`, `devm_of_phy_bulk_get_all`…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch uses non-existent PHY bulk APIs (struct phy_bulk_data, devm_of_phy_bulk_get_all, phy_bulk_init, phy_bulk_exit), which will cause an immediate build failure. --
commit fe08b64098d53a53f4787c86d1612302f802dee7
Author: Inochi Amaoto <[email protected]>
PCI: spacemit-k1: Add multiple PHY handles support
This commit extends the Spacemit K1 PCIe controller driver to support multiple PHY handles by introducing bulk PHY API calls. It replaces single PHY operations with bulk operations throughout the driver's initialization and cleanup paths.
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0@@ -51,12 +51,14 @@ struct k1_pcie_device_data {const struct dw_pcie_host_ops *host_ops;
drivers/pci/controller/dwc/pcie-spacemit-k1.c struct k1_pcie_device_data { · line 60
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +2
-1const 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_bulk_data *phys;
[Severity: Low]
Does struct phy_bulk_data exist in the kernel? It appears this type is undefined, which will cause a build failure here in k1_pcie.
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1
-0+unsigned int phy_count;void __iomem *link;struct regmap *pmu;/* Errors ignored; MMIO-backed regmap */
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_disable_resources(struct k1_pcie *k1) · line 126
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +7
-0@@ -115,6 +117,23 @@ 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; +int count; + + count =devm_of_phy_bulk_get_all (dev, node, &k1->phys);
[Severity: Low]
Is devm_of_phy_bulk_get_all() an upstream API? I'm unable to find this function, and calling an undefined function will break the build during k1_pcie_get_phy_handle().
Quoted 12 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ; + + k1->phy_count = count; + +return 0 ; +} +/* FIXME: Disable ASPM L1 to avoid errors reported on some NVMe drives */ static void k1_pcie_disable_aspm_l1 (struct k1_pcie *k1)
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_init(struct dw_pcie_rp *pp) · line 192
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1
-1@@ -170,7 +189,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_bulk_init (k1->phy_count, k1->phys);
[Severity: Low]
Is phy_bulk_init() defined? Similar to the other bulk APIs used in this patch, this appears to be an undefined function and will fail compilation in k1_pcie_init().
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_deinit(struct dw_pcie_rp *pp) · line 217
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +1
-1if (ret) {k1_pcie_disable_resources (k1);@@ -195,7 +214,7 @@ static void k1_pcie_deinit(struct dw_pcie_rp *pp)regmap_set_bits (k1->pmu, k1->pmu_off +PCIE_CLK_RESET_CONTROL ,PCIE_RC_PERST ); -phy_exit (k1->phy); +phy_bulk_exit (k1->phy_count, k1->phys);
[Severity: Low]
Does phy_bulk_exit() exist? Like the initialization function, this seems to be undefined and will cause compilation to fail during k1_pcie_deinit().
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0k1_pcie_disable_resources (k1); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v5 2/6] PCI: spacemit-k1: Add multiple PHY handles support
In reply to Inochi AmaotoQuoted 3 lines · Inochi Amaoto
The PCIe controller on Spacemit K3 may use multiple PHYs at the same time. The feature is not supported by the current driver. So extend the PHY definition to support multiple PHY handles.
...
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_disable_resources(struct k1_pcie *k1) · line 130
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +11
-0+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; +int count; + + count =devm_of_phy_bulk_get_all (dev, node, &k1->phys); +if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ;
Elsewhere we distinguish count == 0 with -ENOENT.
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ k1->phy_count = count; + +return 0 ; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/6] PCI: spacemit-k1: Add multiple PHY handles support
Quoted 7 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 supported by the current driver. So extend the PHY definition to support multiple PHY handles.
...
Quoted 14 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_disable_resources(struct k1_pcie *k1) · line 130
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +11
-0+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; +int count; + + count =devm_of_phy_bulk_get_all (dev, node, &k1->phys); +if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ;Elsewhere we distinguish count == 0 with
-ENOENT.
Is there is an example? I think it is suitable for its internal check to translate -ENOENT to count 0, because no phys property means no phy. For a get helper with optional semantics. I think it is fine to return 0.
Regards,
Inochi
Quoted 10 lines
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ k1->phy_count = count; + +return 0 ; +}Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/6] PCI: spacemit-k1: Add multiple PHY handles support
Quoted 2 lines · Inochi Amaoto
Quoted 1 line · Andy Shevchenko
On Mon, Sep 07, 2026 at 07:26:01PM +0800, Inochi Amaoto wrote:
...
Quoted 8 lines
Quoted 7 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_disable_resources(struct k1_pcie *k1) · line 130
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+ count =devm_of_phy_bulk_get_all (dev, node, &k1->phys); +if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ;Elsewhere we distinguish count == 0 with
-ENOENT.
Quoted 4 lines
Is there is an example? I think it is suitable for its internal check to translate
-ENOENTto count 0, because no phys property means no phy. For a get helper with optional semantics. I think it is fine to return 0.
This the brief list of what I roughly grepped with word "count" in the name.
gpiod_count()of_hte_req_count()i2c_acpi_resource_count()ice_sriov_set_msix_vec_count()reset_control_get_count()
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/6] PCI: spacemit-k1: Add multiple PHY handles support
Quoted 20 lines · Andy Shevchenko
Quoted 2 lines · Inochi Amaoto
Quoted 1 line · Andy Shevchenko
On Mon, Sep 07, 2026 at 07:26:01PM +0800, Inochi Amaoto wrote:
...
Quoted 8 lines
Quoted 7 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static void k1_pcie_disable_resources(struct k1_pcie *k1) · line 130
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+ count =devm_of_phy_bulk_get_all (dev, node, &k1->phys); +if (count <0 ) +return count; +if (count ==0 || count > data->max_phy_count) +return -EINVAL ;Elsewhere we distinguish count == 0 with
-ENOENT.Quoted 4 lines
Is there is an example? I think it is suitable for its internal check to translate
-ENOENTto count 0, because no phys property means no phy. For a get helper with optional semantics. I think it is fine to return 0.
Quoted 8 lines
This the brief list of what I roughly grepped with word "count" in the name.
gpiod_count()of_hte_req_count()i2c_acpi_resource_count()ice_sriov_set_msix_vec_count()reset_control_get_count()
As the respond in the phy bulk data patchset, devm_of_phy_bulk_get_all() should follow the optional semantics. So I think return 0 is a good choice. Anyway, I have added the zero value check in the driver.
Regards,
Inochi
PATCH v5 3/6 PCI: spacemit-k1: Add device id update helper
In reply to Inochi AmaotoBoth K1 and K3 need 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
@@ -52,6 +52,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 {
@@ -151,6 +152,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);
@@ -166,10 +177,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
@@ -369,6 +377,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 v5 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 need 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 v5 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 v5 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 v5 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 | 50 ++++++++++++++++--- 1 file changed, 43 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +43 -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,43 @@ 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 + + required: + - interrupts + - interrupt-names + 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 v5 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 `spacemit,k3-pcie` compatible string lacks a requirement for any interrupt mechanism…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The spacemit,k3-pcie compatible string lacks a requirement for any interrupt mechanism, allowing incomplete device tree nodes to silently pass schema validation. --
commit 92cb9c20a16a6dc1a934eea83a6f78a972ade969
Author: Inochi Amaoto <[email protected]>
dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
This commit adds device tree binding support for the SpacemiT K3 PCIe controller, modifying the existing K1 binding. It splits register constraints and interrupt requirements between the two hardware generations.
Quoted 4 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-0
[ ... ]
Quoted 8 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +8
-0+allOf: + - $ref: /schemas/pci/snps,dw-pcie.yaml# + - if: + properties: + compatible: + contains: + const: spacemit,k1-pcie + then:
[ ... ]
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml patternProperties: · line 135
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +10
-0+ required: + - interrupts + - interrupt-names + else: + properties: + reg: + minItems: 5 + + reg-names: + minItems: 5
[Severity: Medium]
Does the spacemit,k3-pcie compatible lack a requirement for an interrupt mechanism here?
Because this patch removes interrupts and interrupt-names from the global required list (moved to the then block above), a device tree node for spacemit,k3-pcie that completely lacks interrupt or MSI routing properties like msi-parent will silently pass schema validation.
However, the system will fail at runtime because the PCIe host controller requires an interrupt mechanism. Should a required list including msi-parent be added to this else block to ensure incomplete device tree nodes are caught during validation?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Troy Mitchell
reviewerRe: [PATCH v5 5/6] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
In reply to Inochi AmaotoQuoted 4 lines · Inochi Amaoto
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-0@@ -14,26 +14,29 @@ description: > PCIe IP. The controller uses the DesignWare built-in MSI interrupt controller, and supports 256 MSIs.
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml description: > · line 22
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +3
-4-allOf: - - $ref: /schemas/pci/snps,dw-pcie.yaml# - properties: compatible: - const: spacemit,k1-pcie + enum: + - spacemit,k1-pcie + - spacemit,k3-pcie
Please update the title and description to cover K3 as well. They still describe only K1.
- Troy
Re: [PATCH v5 5/6] dt-bindings: PCI: spacemit: Introduce Spacemit K3 PCIe host controller
Quoted 19 lines · Troy Mitchell
Quoted 4 lines · Inochi Amaoto
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +0
-0@@ -14,26 +14,29 @@ description: > PCIe IP. The controller uses the DesignWare built-in MSI interrupt controller, and supports 256 MSIs.Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml description: > · line 22
Quoted 10 lines
Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml +3
-4-allOf: - - $ref: /schemas/pci/snps,dw-pcie.yaml# - properties: compatible: - const: spacemit,k1-pcie + enum: + - spacemit,k1-pcie + - spacemit,k3-pciePlease update the title and description to cover K3 as well. They still describe only K1.
Quoted 1 line
- Troy
Right, I forgot this. Thanks for the reminder.
Regards,
Inochi
PATCH v5 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 | 132 ++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/Kconfig +2 -2
@@ -443,7 +443,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
@@ -451,7 +451,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 +132 -0
160 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v5 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] Incorrect PCIe Gen3 Equalization Preset Request Vector assignment. - [High] The driver accesses…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect PCIe Gen3 Equalization Preset Request Vector assignment.
- [High] The driver accesses link MMIO registers before peripheral clocks are enabled, causing a bus fault.
- [Medium] The error path in k3_pcie_init incorrectly de-asserts PERST#, failing to safely reset the device.
- [Low] Use of undefined PHY bulk API functions causing build failure.
--
commit 63c719f7a87ffea307709340b4548f687247044d
Author: Inochi Amaoto <[email protected]>
Subject: PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
This commit adds driver support for the PCIe controller on the Spacemit K3 SoC. The controller is similar to the Synopsys DesignWare PCIe IP but adds support for an external MSI interrupt controller and multiple PHYs.
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +0
-0
[ ... ]
Quoted 19 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +15
-0@@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)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); + +/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */ + regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC, + PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT,
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 343
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE); + + ret =k1_pcie_enable_resources (k1); +if (ret) + goto failed_resources; + +regmap_set_bits (k1->pmu, reset_ctrl,PCIE_AUX_PWR_DET ); +regmap_clear_bits (k1->pmu, reset_ctrl,APP_HOLD_PHY_RST ); + + ret =phy_bulk_init (k1->phy_count, k1->phys);
[Severity: Low]
Does phy_bulk_init() exist in the mainline kernel? It seems this API might be missing, which could cause a build failure here.
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 354
Quoted 11 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +11
-0+if (ret) + goto failed_phy; + +msleep (PCIE_T_PVPERL_MS ); + +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,BIT (7 ), +GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC );
[Severity: High]
Does this assignment set the correct equalization preset? BIT(7) evaluates to 128 (0x80), which when inserted into the 16-bit preset request vector field, assigns preset 8 to Lane 1 and 0 to all other lanes. Could this cause PCIe Gen3 link training to fail or use incorrect presets?
Quoted 5 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+dw_pcie_writel_dbi (pci,GEN3_EQ_CONTROL_OFF , val); + +k1_pcie_set_device_id (k1); + +/* Finally, as a workaround, disable ASPM L1 */
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 369
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+k1_pcie_disable_aspm_l1 (k1); + +return 0 ; + +failed_phy: +k1_pcie_disable_resources (k1); +failed_resources: +regmap_update_bits (k1->pmu, k1->pmu_off +PCIE_CONTROL_LOGIC , +PCIE_PERSTN_OUT |PCIE_PERSTN_OE |PCIE_IGNORE_PERSTN , +PCIE_PERSTN_OUT |PCIE_PERSTN_OE );
[Severity: Medium]
In this error path, PCIE_PERSTN_OUT is set high, which appears to de-assert PERST#. Should this be clearing PCIE_PERSTN_OUT instead, to ensure the endpoint device is safely held in reset after initialization fails?
Quoted 3 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +3
-0+ +return ret; +}
[ ... ]
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 418
Quoted 6 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +6
-0+static int k3_pcie_parse_port (struct k1_pcie *k1) +{ +u32 status0, status1, status2; + +/* This register require a RAW for cleanup */ + status0 =readl_relaxed (k1->link +K3_PHY_AHB_IRQSTATUS_INTX );
[Severity: High]
Can this unclocked read cause a bus hang or synchronous external abort? k3_pcie_parse_port() is called unconditionally during k1_pcie_probe():
k1_pcie_probe()
data->parse_port() -> k3_pcie_parse_port()
...
dw_pcie_host_init()
k3_pcie_init()
k1_pcie_enable_resources()It looks like the peripheral clocks are not enabled until k1_pcie_enable_resources() is called later in the host initialization. Reading the k1->link registers before the clocks are enabled might fault.
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +9
-0+ 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 ); + +return k1_pcie_parse_port (k1); +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Troy Mitchell
reviewerRe: [PATCH v5 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
In reply to Inochi AmaotoQuoted 28 lines · Inochi Amaoto
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +23
-0@@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1) +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); + +/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */ +regmap_update_bits (k1->pmu, k1->pmu_off +PCIE_CONTROL_LOGIC , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE |PCIE_PERSTN_OUT , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE ); + + ret =k1_pcie_enable_resources (k1); +if (ret) + goto failed_resources; + +regmap_set_bits (k1->pmu, reset_ctrl,PCIE_AUX_PWR_DET ); +regmap_clear_bits (k1->pmu, reset_ctrl,APP_HOLD_PHY_RST );
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 351
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ + ret =phy_bulk_init (k1->phy_count, k1->phys); +if (ret) + goto failed_phy; + +msleep (PCIE_T_PVPERL_MS ); + +regmap_set_bits (k1->pmu, k1->pmu_off +PCIE_CONTROL_LOGIC , +PCIE_PERSTN_OUT |PCIE_PERSTN_OE ); +
Should we use pci->pe_rst when reset-gpios is provided, and keep the PMU path as a fallback? The SDK handles both cases. The DWC core requests that GPIO with GPIOD_OUT_HIGH, but this path only releases PERST# through the PMU, so an endpoint using the GPIO would remain in reset.
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 363
Quoted 7 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+/* Finally, as a workaround, disable ASPM L1 */ +k1_pcie_disable_aspm_l1 (k1); + +return 0 ; +
Would we also need to configure IOMMU bypass during initialization? The SDK sets the PCIe A/B/C bypass bits in PMUA_PCIE_SUBSYS_MGMT when there is no usable iommu-map. The proposed K3 PCIe DTS has no iommu-map, and I could not find the corresponding bypass setup in this series.
Is bypass already guaranteed by firmware or the reset state, or should the driver set it here? My concern is that enumeration could succeed while endpoint DMA still goes through an unconfigured IOMMU.
Quoted 8 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +6
-0+static int k3_pcie_parse_port (struct k1_pcie *k1) +{ +u32 status0, status1, status2; + +/* This register require a RAW for cleanup */ + status0 =readl_relaxed (k1->link +K3_PHY_AHB_IRQSTATUS_INTX );
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 428
Quoted 10 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +10
-0+ 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 ); + +return k1_pcie_parse_port (k1); +} +
Are these status registers accessible before the controller clocks are enabled and resets released? k3_pcie_parse_port() runs before dw_pcie_host_init(), which calls k3_pcie_init() to enable those resources.
The SDK uses the same ordering, but I am not sure whether it relies on firmware leaving the registers accessible. If so, would it be safer to move this clearing into k3_pcie_init(), after enabling the resources?
- Troy
Re: [PATCH v5 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
Quoted 36 lines · Troy Mitchell
Quoted 35 lines · Inochi Amaoto
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +30
-0@@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1) +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); + +/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */ +regmap_update_bits (k1->pmu, k1->pmu_off +PCIE_CONTROL_LOGIC , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE |PCIE_PERSTN_OUT , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE ); + + ret =k1_pcie_enable_resources (k1); +if (ret) + goto failed_resources; + +regmap_set_bits (k1->pmu, reset_ctrl,PCIE_AUX_PWR_DET ); +regmap_clear_bits (k1->pmu, reset_ctrl,APP_HOLD_PHY_RST ); + + ret =phy_bulk_init (k1->phy_count, k1->phys); +if (ret) + goto failed_phy; + +msleep (PCIE_T_PVPERL_MS ); +
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 351
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_PERSTN_OUT |PCIE_PERSTN_OE ); +Should we use
pci->pe_rstwhen reset-gpios is provided, and keep the PMU path as a fallback? The SDK handles both cases. The DWC core requests that GPIO withGPIOD_OUT_HIGH, but this path only releases PERST# through the PMU, so an endpoint using the GPIO would remain in reset.
I do not think this should be included in this version. I found PICO-ITX has no reset gpio support. This means I can not test this feature.
I suggest adding this function when there is a board using this function.
Quoted 13 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 363
Quoted 7 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+/* Finally, as a workaround, disable ASPM L1 */ +k1_pcie_disable_aspm_l1 (k1); + +return 0 ; +Would we also need to configure IOMMU bypass during initialization? The SDK sets the PCIe A/B/C bypass bits in
PMUA_PCIE_SUBSYS_MGMTwhen there is no usable iommu-map. The proposed K3 PCIe DTS has no iommu-map, and I could not find the corresponding bypass setup in this series.
Quoted 4 lines
Is bypass already guaranteed by firmware or the reset state, or should the driver set it here? My concern is that enumeration could succeed while endpoint DMA still goes through an unconfigured IOMMU.
I think the firmware should mark it bypassed as the default, at least I have notice this behavior, but I am not sure whether it is the builtin firmware or the uboot do this trick.
Quoted 14 lines
Quoted 14 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +12
-0+static int k3_pcie_parse_port (struct k1_pcie *k1) +{ +u32 status0, status1, status2; + +/* This register require a RAW for cleanup */ + 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 );
Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 428
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ +return k1_pcie_parse_port (k1); +} +Are these status registers accessible before the controller clocks are enabled and resets released?
k3_pcie_parse_port()runs beforedw_pcie_host_init(), which callsk3_pcie_init()to enable those resources.
Yes they can. It is something interesting.
Quoted 4 lines
The SDK uses the same ordering, but I am not sure whether it relies on firmware leaving the registers accessible. If so, would it be safer to move this clearing into
k3_pcie_init(), after enabling the resources?
In fact, I have no idea about which clock control this MMIO area, if it is dbi clock (but I guest it is not), it is kind of weird for this clear and should move to the init. Do you have some knowledge on this?
Regards,
Inochi
Troy Mitchell
reviewerRe: [PATCH v5 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
Quoted 47 lines · Inochi Amaoto
Quoted 36 lines · Troy Mitchell
Quoted 35 lines · Inochi Amaoto
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +30
-0@@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1) +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); + +/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */ +regmap_update_bits (k1->pmu, k1->pmu_off +PCIE_CONTROL_LOGIC , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE |PCIE_PERSTN_OUT , +PCIE_IGNORE_PERSTN |PCIE_PERSTN_OE ); + + ret =k1_pcie_enable_resources (k1); +if (ret) + goto failed_resources; + +regmap_set_bits (k1->pmu, reset_ctrl,PCIE_AUX_PWR_DET ); +regmap_clear_bits (k1->pmu, reset_ctrl,APP_HOLD_PHY_RST ); + + ret =phy_bulk_init (k1->phy_count, k1->phys); +if (ret) + goto failed_phy; + +msleep (PCIE_T_PVPERL_MS ); +Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 351
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_PERSTN_OUT |PCIE_PERSTN_OE ); +Should we use
pci->pe_rstwhen reset-gpios is provided, and keep the PMU path as a fallback? The SDK handles both cases. The DWC core requests that GPIO withGPIOD_OUT_HIGH, but this path only releases PERST# through the PMU, so an endpoint using the GPIO would remain in reset.
Quoted 4 lines
I do not think this should be included in this version. I found PICO-ITX has no reset gpio support. This means I can not test this feature.
I suggest adding this function when there is a board using this function.
Agreed, we can defer GPIO reset support until a board needs it and we can test it. I checked again, and the SDK only added this support recently. It provides GPIO-controlled PERST# as an alternative to the native PERST# control through the PMU.
Quoted 14 lines
Quoted 14 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 363
Quoted 8 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +5
-0+/* Finally, as a workaround, disable ASPM L1 */ +k1_pcie_disable_aspm_l1 (k1); + +return 0 ; +Would we also need to configure IOMMU bypass during initialization? The SDK sets the PCIe A/B/C bypass bits in
PMUA_PCIE_SUBSYS_MGMTwhen there is no usable iommu-map. The proposed K3 PCIe DTS has no iommu-map, and I could not find the corresponding bypass setup in this series.
Quoted 8 lines
Quoted 4 lines
Is bypass already guaranteed by firmware or the reset state, or should the driver set it here? My concern is that enumeration could succeed while endpoint DMA still goes through an unconfigured IOMMU.
I think the firmware should mark it bypassed as the default, at least I have notice this behavior, but I am not sure whether it is the builtin firmware or the uboot do this trick.
I checked the register specification. The PCIe A/B/C IOMMU bypass bits reset to 1, so bypass is the hardware reset default and does not require firmware to enable it. That resolves my concern.
Quoted 27 lines
Quoted 15 lines
Quoted 15 lines
[...]
drivers/pci/controller/dwc/pcie-spacemit-k1.c +12
-0+static int k3_pcie_parse_port (struct k1_pcie *k1) +{ +u32 status0, status1, status2; + +/* This register require a RAW for cleanup */ + 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 );Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 428
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ +return k1_pcie_parse_port (k1); +} +Are these status registers accessible before the controller clocks are enabled and resets released?
k3_pcie_parse_port()runs beforedw_pcie_host_init(), which callsk3_pcie_init()to enable those resources.Yes they can. It is something interesting.
Quoted 8 lines
Quoted 4 lines
The SDK uses the same ordering, but I am not sure whether it relies on firmware leaving the registers accessible. If so, would it be safer to move this clearing into
k3_pcie_init(), after enabling the resources?In fact, I have no idea about which clock control this MMIO area, if it is dbi clock (but I guest it is not), it is kind of weird for this clear and should move to the init. Do you have some knowledge on this?
I have not confirmed which clock controls this MMIO region yet. I have asked our clock team about its clock and reset dependencies and whether access before resource initialization is guaranteed. I will follow up once I have clarification.
Quoted 3 lines
Regards,
Inochi
Signature · 1 line
Troy Mitchell
Troy Mitchell
reviewerRe: [PATCH v5 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
In reply to Inochi AmaotoQuoted 26 lines · Inochi Amaoto
[...]
Quoted 12 lines
Quoted 12 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +12
-0+static int k3_pcie_parse_port (struct k1_pcie *k1) +{ +u32 status0, status1, status2; + +/* This register require a RAW for cleanup */ + 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 );Quoted 9 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c static int k1_pcie_parse_port(struct k1_pcie *k1) · line 428
Quoted 4 lines
drivers/pci/controller/dwc/pcie-spacemit-k1.c +4
-0+ +return k1_pcie_parse_port (k1); +} +Are these status registers accessible before the controller clocks are enabled and resets released?
k3_pcie_parse_port()runs beforedw_pcie_host_init(), which callsk3_pcie_init()to enable those resources.Yes they can. It is something interesting.
Quoted 8 lines
Quoted 4 lines
The SDK uses the same ordering, but I am not sure whether it relies on firmware leaving the registers accessible. If so, would it be safer to move this clearing into
k3_pcie_init(), after enabling the resources?In fact, I have no idea about which clock control this MMIO area, if it is dbi clock (but I guest it is not), it is kind of weird for this clear and should move to the init. Do you have some knowledge on this?
I checked with our hardware team. PMU AP and PCIECFG share a system clock derived from PLL1 /8 or /6.
This clock is already available during early boot, before PCIe controller initialization. The register accesses in k3_pcie_parse_port() therefore do not need to wait for k3_pcie_init(). That resolves my concern about the ordering.
- Troy