[PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Alex Elder]

This series introduces a PHY driver and a PCIe driver to support PCIe
on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
one PCIe lane, and the other two ports each have two lanes.  All PCIe
ports operate at 5 GT/second.

The PCIe PHYs must be configured using a value that can only be
determined using the combo PHY, operating in PCIe mode.  To allow
that PHY to be used for USB, the calibration step is performed by
the PHY driver automatically at probe time.  Once this step is done,
the PHY can be used for either PCIe or USB.

Version 2 of this series incorporates suggestions made during the
review of version 1.  Specific highlights are detailed below.

					-Alex

This series is available here:
  https://github.com/riscstar/linux/tree/outgoing/pcie-v2

Between version 1 and version 2:
  - General
    - VENDOR ID 0x201f is now registered with PCI SIG: "SpacemiT
      (Hangzhou) Technology Co. Ltd"
        https://pcisig.com/membership/member-companies?combine=201f
    - The PCIe host compatible string is now "spacemit,k1-pcie"
    - Reimplemented the PHY PLL as a clock registered with the
      common clock framework, driven by an external oscillator
    - Added the external oscillator clock to the PHY binding
    - Renamed the PCIe driver source file "pcie-spacemit-k1.c"
  - Kconfig
    - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1
    - The PCIe driver is now defined as tristate, not Boolean
    - Updated the PCIe host Kconfig based on Bjorn H's feedback
  - DT Bindings
    - Corrected PCIe node ranges properties
    - Replaced "interrupts-extended" property with just "interrupts"
      in the PCIe host binding
    - Named the single PCIe interrupt "msi" to clarify its purpose
    - Added a new vpcie3v3-supply property for PCIe ports
    - Renamed a syscon property to align with other SpacemiT bindings
    - Removed labels and status properties in DT binding examples
    - Added a '>' to DT binding descriptions to preserve formatting
    - Consistently ended DT binding descriptions with no period
    - Dropped ".yaml" from the PCIe host compatible string
    - Dropped unneeded max-link-speed property from PCIe binding,
      relying on the hardware default value instead
    - No longer require the bus-ranges PCIe property; if not
      provided, the default value used is exactly what's desired
  - Code
    - Renamed the symbols representing the PCI vendor and device IDs
      to align with <linux/pci_ids.h>
    - Use PCIE_T_PVPERL_MS rather than 100 to represent a standard
      delay period.
    - Use platform (not dev) driver-data access functions; assignment
      is done only after the private structure is initialized
    - Deleted some unneeded includes in the PCIe driver.
    - Dropped error checking when operating on MMIO-backed regmaps
    - Added a regmap_read() call in two places, to ensure a specified
      delay occurs *after* the a MMIO write has reached its target.
    - Used ARRAY_SIZE() (not a local variable value) in a few spots
    - Now use readl_relaxed()/writel_relaxed() when operating on
      the "link" I/O memory space in the PCIe driver
    - Updated a few error messages for consistency
    - No longer specify suppress_bind_attrs in the PCIe driver
    - Now specify PCIe driver probe type as PROBE_PREFER_ASYNCHRONOUS
  - Miscellany
    - Subject on the PCIe host binding includes "pci", not "phy"
    - Clarified that the DesignWare built-in MSI controller is used
    - Use "PCIe gen2" terminology (rather than "PCIe v2")
    - No longer use (void) cast to indicate ignored return values

Here is version 1 of this series:
  https://lore.kernel.org/lkml/20250813184701.2444372-1-elder@riscstar.com/


Alex Elder (7):
  dt-bindings: phy: spacemit: add SpacemiT PCIe/combo PHY
  dt-bindings: phy: spacemit: introduce PCIe PHY
  dt-bindings: pci: spacemit: introduce PCIe host controller
  phy: spacemit: introduce PCIe/combo PHY
  PCI: spacemit: introduce SpacemiT PCIe host driver
  riscv: dts: spacemit: add a PCIe regulator
  riscv: dts: spacemit: PCIe and PHY-related updates

 .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++
 .../bindings/phy/spacemit,k1-combo-phy.yaml   | 114 +++
 .../bindings/phy/spacemit,k1-pcie-phy.yaml    |  59 ++
 .../boot/dts/spacemit/k1-bananapi-f3.dts      |  38 +
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  |  33 +
 arch/riscv/boot/dts/spacemit/k1.dtsi          | 151 ++++
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 319 +++++++++
 drivers/phy/Kconfig                           |  11 +
 drivers/phy/Makefile                          |   1 +
 drivers/phy/phy-spacemit-k1-pcie.c            | 672 ++++++++++++++++++
 12 files changed, 1565 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-spacemit-k1.c
 create mode 100644 drivers/phy/phy-spacemit-k1-pcie.c


base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
-- 
2.48.1

[PATCH v2 1/7] dt-bindings: phy: spacemit: add SpacemiT PCIe/combo PHY

[Alex Elder]

Add the Device Tree binding for the PCIe/USB 3.0 combo PHY found in
the SpacemiT K1 SoC.  This is one of three PCIe PHYs, and is unusual
in that only the combo PHY can perform a calibration step needed to
determine settings used by the other two PCIe PHYs.

Calibration must be done with the combo PHY in PCIe mode, and to allow
this to occur independent of the eventual use for the PHY (PCIe or USB)
some PCIe-related properties must be supplied: clocks; resets; and a
syscon phandle.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Added '>' to the description, and reworded it a bit
    - Added an external oscillator clock, "refclk"
    - Renamed the "global" reset to be "phy"
    - Renamed a phandle property to be "spacemit,apmu"
    - Dropped the label and status property from the example

 .../bindings/phy/spacemit,k1-combo-phy.yaml   | 114 ++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml b/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
new file mode 100644
index 0000000000000..6e2f401b0ac27
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/spacemit,k1-combo-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 PCIe/USB3 Combo PHY
+
+maintainers:
+  - Alex Elder <elder@riscstar.com>
+
+description: >
+  Of the three PHYs on the SpacemiT K1 SoC capable of being used for
+  PCIe, one is a combo PHY that can also be configured for use by a
+  USB 3 controller.  Using PCIe or USB 3 is a board design decision.
+
+  The combo PHY is also the only PCIe PHY that is able to determine
+  PCIe calibration values to use, and this must be determined before
+  the other two PCIe PHYs can be used.  This calibration must be
+  performed with the combo PHY in PCIe mode, and is this is done
+  when the combo PHY is probed.
+
+  The combo PHY uses an external oscillator as a reference clock.
+  During normal operation, the PCIe or USB port driver is responsible
+  for ensuring all other clocks needed by a PHY are enabled, and all
+  resets affecting the PHY are deasserted.  However, for the combo
+  PHY to perform calibration independent of whether it's later used
+  for PCIe or USB, all PCIe mode clocks and resets must be defined.
+
+properties:
+  compatible:
+    const: spacemit,k1-combo-phy
+
+  reg:
+    items:
+      - description: PHY control registers
+
+  clocks:
+    items:
+      - description: External oscillator used by the PHY PLL
+      - 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: refclk
+      - 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
+      - description: PHY reset; must be deasserted for PHY to function
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+      - const: phy
+
+  spacemit,apmu:
+    description:
+      A phandle that refers to the APMU system controller, whose
+      regmap is used in setting the mode
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  "#phy-cells":
+    const: 1
+    description:
+      The argument value (PHY_TYPE_PCIE or PHY_TYPE_USB3) determines
+      whether the PHY operates in PCIe or USB3 mode.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - spacemit,apmu
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/spacemit,k1-syscon.h>
+    phy@c0b10000 {
+        compatible = "spacemit,k1-combo-phy";
+        reg = <0xc0b10000 0x1000>;
+        clocks = <&vctcxo_24m>,
+                 <&syscon_apmu CLK_PCIE0_DBI>,
+                 <&syscon_apmu CLK_PCIE0_MASTER>,
+                 <&syscon_apmu CLK_PCIE0_SLAVE>;
+        clock-names = "refclk",
+                      "dbi",
+                      "mstr",
+                      "slv";
+        resets = <&syscon_apmu RESET_PCIE0_DBI>,
+                 <&syscon_apmu RESET_PCIE0_MASTER>,
+                 <&syscon_apmu RESET_PCIE0_SLAVE>,
+                 <&syscon_apmu RESET_PCIE0_GLOBAL>;
+        reset-names = "dbi",
+                      "mstr",
+                      "slv",
+                      "phy";
+        spacemit,apmu = <&syscon_apmu>;
+        #phy-cells = <1>;
+    };
-- 
2.48.1

[PATCH v2 2/7] dt-bindings: phy: spacemit: introduce PCIe PHY

[Alex Elder]

Add the Device Tree binding for two PCIe PHYs present on the SpacemiT
K1 SoC.  These PHYs are dependent on a separate combo PHY, which
determines at probe time the calibration values used by the PCIe-only
PHYs.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Added '>' to the description, and reworded it a bit
    - Added clocks and clock-names properties
    - Dropped the label and status property from the example

 .../bindings/phy/spacemit,k1-pcie-phy.yaml    | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml
new file mode 100644
index 0000000000000..c821f6cafcdae
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/spacemit,k1-pcie-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 PCIe PHY
+
+maintainers:
+  - Alex Elder <elder@riscstar.com>
+
+description: >
+  There are two PHYs on the SpacemiT K1 SoC used for PCIe (only).
+  These PHYs must be configured using calibration values that are
+  determined by a third "combo PHY".  The combo PHY determines
+  these calibration values during probe so they can be used for
+  the two PCIe-only PHYs.
+
+  The PHY uses an external oscillator as a reference clock.  During
+  normal operation, the PCIe host driver is responsible for ensuring
+  all other clocks needed by a PHY are enabled, and all resets
+  affecting the PHY are deasserted.
+
+properties:
+  compatible:
+    const: spacemit,k1-pcie-phy
+
+  reg:
+    items:
+      - description: PHY control registers
+
+  clocks:
+    items:
+      - description: External oscillator used by the PHY PLL
+
+  clock-names:
+    const: refclk
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    phy@c0c10000 {
+        compatible = "spacemit,k1-pcie-phy";
+        reg = <0xc0c10000 0x1000>;
+        clocks = <&vctcxo_24m>;
+        clock-names = "refclk";
+        #phy-cells = <0>;
+    };
-- 
2.48.1

[PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Alex Elder]

Add the Device Tree binding for the PCIe root complex found on the
SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
typically used to support a USB 3 port.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Renamed the binding, using "host controller"
    - Added '>' to the description, and reworded it a bit
    - Added reference to /schemas/pci/snps,dw-pcie.yaml
    - Fixed and renamed the compatible string
    - Renamed the PMU property, and fixed its description
    - Consistently omit the period at the end of descriptions
    - Renamed the "global" clock to be "phy"
    - Use interrupts rather than interrupts-extended, and name the
      one interrupt "msi" to make clear its purpose
    - Added a vpcie3v3-supply property
    - Dropped the max-link-speed property
    - Changed additionalProperties to unevaluatedProperties
    - Dropped the label and status property from the example

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

diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
new file mode 100644
index 0000000000000..87745d49c53a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
@@ -0,0 +1,156 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 PCI Express Host Controller
+
+maintainers:
+  - Alex Elder <elder@riscstar.com>
+
+description: >
+  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
+  DesignWare 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
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: ATU address space
+      - description: PCIe configuration space
+      - description: Link control registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: atu
+      - const: config
+      - const: link
+
+  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
+
+  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
+      - description: Global reset; must be deasserted for PHY to function
+
+  reset-names:
+    items:
+      - const: dbi
+      - const: mstr
+      - const: slv
+      - const: phy
+
+  interrupts:
+    items:
+      - description: Interrupt used for MSIs
+
+  interrupt-names:
+    const: msi
+
+  phys:
+    maxItems: 1
+
+  vpcie3v3-supply:
+    description:
+      A phandle for 3.3v regulator to use for PCIe
+
+  device_type:
+    const: pci
+
+  num-viewport:
+    const: 8
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - spacemit,apmu
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - interrupts
+  - interrupt-names
+  - phys
+  - vpcie3v3-supply
+  - device_type
+  - num-viewport
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/spacemit,k1-syscon.h>
+    pcie@ca400000 {
+        compatible = "spacemit,k1-pcie";
+        reg = <0xca400000 0x00001000>,
+              <0xca700000 0x0001ff24>,
+              <0x9f000000 0x00002000>,
+              <0xc0c20000 0x00001000>;
+        reg-names = "dbi",
+                    "atu",
+                    "config",
+                    "link";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x01000000 0x0 0x00000000 0x9f002000 0x0 0x00100000>,
+                 <0x02000000 0x0 0x90000000 0x90000000 0x0 0x0f000000>;
+        interrupts = <142>;
+        interrupt-names = "msi";
+        clocks = <&syscon_apmu CLK_PCIE1_DBI>,
+                 <&syscon_apmu CLK_PCIE1_MASTER>,
+                 <&syscon_apmu CLK_PCIE1_SLAVE>;
+        clock-names = "dbi",
+                      "mstr",
+                      "slv";
+        resets = <&syscon_apmu RESET_PCIE1_DBI>,
+                 <&syscon_apmu RESET_PCIE1_MASTER>,
+                 <&syscon_apmu RESET_PCIE1_SLAVE>,
+                 <&syscon_apmu RESET_PCIE1_GLOBAL>;
+        reset-names = "dbi",
+                      "mstr",
+                      "slv",
+                      "phy";
+        phys = <&pcie1_phy>;
+        vpcie3v3-supply = <&pcie_vcc_3v3>;
+        device_type = "pci";
+        num-viewport = <8>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pcie1_3_cfg>;
+        spacemit,apmu = <&syscon_apmu 0x3d4>;
+    };
-- 
2.48.1

[PATCH v2 4/7] phy: spacemit: introduce PCIe/combo PHY

[Alex Elder]

Introduce a driver that supports three PHYs found on the SpacemiT
K1 SoC.  The first PHY is a combo PHY that can be configured for
use for either USB 3 or PCIe.  The other two PHYs support PCIe
only.

All three PHYs must be programmed with an 8 bit receiver termination
value, which must be determined dynamically.  Only the combo PHY is
able to determine this value.  The combo PHY performs a special
calibration step at probe time to discover this, and that value is
used to program each PHY that operates in PCIe mode.  The combo
PHY must therefore be probed before either of the PCIe-only PHYs
will be used.

Each PHY has an internal PLL driven from an external oscillator.
This PLL started when the PHY is first initialized, and stays
on thereafter.

During normal operation, the USB or PCIe driver using the PHY must
ensure (other) clocks and resets are set up properly.

However PCIe mode clocks are enabled and resets are de-asserted
temporarily by this driver to perform the calibration step on the
combo PHY.

Tested-by: Junzhong Pan <panjunzhong@linux.spacemit.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1
    - Reimplemented the PHY PLL as a clock registered with the
      common clock framework, driven by an external oscillator
    - Memory-mapped regmap operations no longer check for errors
    - Bulk clocks are now named, allowing the PLL clock to be
      managed separate from the rest
    - No longer use a "virt" local variable for read/modify/write
    - Deleted a few unused symbol definitions
    - Added and reworded some comments

 drivers/phy/Kconfig                |  11 +
 drivers/phy/Makefile               |   1 +
 drivers/phy/phy-spacemit-k1-pcie.c | 672 +++++++++++++++++++++++++++++
 3 files changed, 684 insertions(+)
 create mode 100644 drivers/phy/phy-spacemit-k1-pcie.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 678dd0452f0aa..1984c2e56122e 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -101,6 +101,17 @@ config PHY_NXP_PTN3222
 	  schemes. It supports all three USB 2.0 data rates: Low Speed, Full
 	  Speed and High Speed.
 
+config PHY_SPACEMIT_K1_PCIE
+	tristate "PCIe and combo PHY driver for the SpacemiT K1 SoC"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	select GENERIC_PHY
+	default ARCH_SPACEMIT
+	help
+	  Enable support for the PCIe and USB 3 combo PHY and two
+	  PCIe-only PHYs used in the SpacemiT K1 SoC.
+
 source "drivers/phy/allwinner/Kconfig"
 source "drivers/phy/amlogic/Kconfig"
 source "drivers/phy/broadcom/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index bfb27fb5a4942..a206133a35151 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PHY_SNPS_EUSB2)		+= phy-snps-eusb2.o
 obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
 obj-$(CONFIG_PHY_AIROHA_PCIE)		+= phy-airoha-pcie.o
 obj-$(CONFIG_PHY_NXP_PTN3222)		+= phy-nxp-ptn3222.o
+obj-$(CONFIG_PHY_SPACEMIT_K1_PCIE)	+= phy-spacemit-k1-pcie.o
 obj-y					+= allwinner/	\
 					   amlogic/	\
 					   broadcom/	\
diff --git a/drivers/phy/phy-spacemit-k1-pcie.c b/drivers/phy/phy-spacemit-k1-pcie.c
new file mode 100644
index 0000000000000..81bc05823d080
--- /dev/null
+++ b/drivers/phy/phy-spacemit-k1-pcie.c
@@ -0,0 +1,672 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SpacemiT K1 PCIe and PCIe/USB 3 combo PHY driver
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <dt-bindings/phy/phy.h>
+
+/*
+ * Three PCIe ports are supported in the SpacemiT K1 SoC, and this driver
+ * supports their PHYs.
+ *
+ * The PHY for PCIe port A is different from the PHYs for ports B and C:
+ * - It has one lane, while ports B and C have two
+ * - It is a combo PHY can be used for PCIe or USB 3
+ * - It can automatically calibrate PCIe TX and RX termination settings
+ *
+ * The PHY functionality for PCIe ports B and C is identical:
+ * - They have two PCIe lanes (but can be restricted to 1 via Device Tree)
+ * - They are used for PCIe only
+ * - They are configured using TX and RX values computed for port A
+ *
+ * A given board is designed to use the combo PHY for either PCIe or USB 3.
+ * Whether the combo PHY is configured for PCIe or USB 3 is specified in
+ * Device Tree using a phandle plus an argument.  The argument indicates
+ * the type (either PHY_TYPE_PCIE or PHY_TYPE_USB3).
+ *
+ * Each PHY depends on clocks and resets provided by the controller
+ * hardware (PCIe or USB) it is associated with.  The controller drivers
+ * are required to enable any clocks and de-assert any resets that affect
+ * PHY operation.  In addition each PHY implements an internal PLL, driven
+ * by an external (24 MHz) oscillator.
+ *
+ * PCIe PHYs must be programmed with RX and TX calibration values.  The
+ * combo PHY is the only one that can determine these values.  They are
+ * determined by temporarily enabling the combo PHY in PCIe mode at probe
+ * time (if necessary).  This calibration only needs to be done once, and
+ * when it has completed the TX and RX values are saved.
+ *
+ * To allow the combo PHY to be enabled for calibration, the resets and
+ * clocks it uses in PCIe mode must be supplied.
+ */
+
+struct k1_pcie_phy {
+	struct device *dev;		/* PHY provider device */
+	struct phy *phy;
+	void __iomem *regs;
+	u32 pcie_lanes;			/* Max (1 or 2) unless limited by DT */
+	struct clk *pll;
+	struct clk_hw pll_hw;		/* Private PLL clock */
+
+	/* The remaining fields are only used for the combo PHY */
+	u32 type;			/* PHY_TYPE_PCIE or PHY_TYPE_USB3 */
+	struct regmap *pmu;		/* MMIO regmap (no errors) */
+};
+
+#define CALIBRATION_TIMEOUT		500000	/* For combo PHY (usec) */
+#define PLL_TIMEOUT			500000	/* For PHY PLL lock (usec) */
+#define POLL_DELAY			500	/* Time between polls (usec) */
+
+/* Selecting the combo PHY operating mode requires APMU regmap access */
+#define SYSCON_APMU			"spacemit,apmu"
+
+/* PMU space, for selecting between PCIe and USB 3 mode (combo PHY only) */
+
+#define PMUA_USB_PHY_CTRL0			0x0110
+#define COMBO_PHY_SEL			BIT(3)	/* 0: PCIe; 1: USB 3 */
+
+#define PCIE_CLK_RES_CTRL			0x03cc
+#define PCIE_APP_HOLD_PHY_RST		BIT(30)
+
+/* PHY register space */
+
+/* Offset between lane 0 and lane 1 registers when there are two */
+#define PHY_LANE_OFFSET				0x0400
+
+/* PHY PLL configuration */
+#define PCIE_PU_ADDR_CLK_CFG			0x0008
+#define PLL_READY			BIT(0)		/* read-only */
+#define CFG_INTERNAL_TIMER_ADJ		GENMASK(10, 7)
+#define TIMER_ADJ_USB		0x2
+#define TIMER_ADJ_PCIE		0x6
+#define CFG_SW_PHY_INIT_DONE		BIT(11)	/* We set after PLL config */
+
+#define PCIE_RC_DONE_STATUS			0x0018
+#define CFG_FORCE_RCV_RETRY		BIT(10)		/* Used for PCIe */
+
+/* PCIe PHY lane calibration; assumes 24MHz input clock */
+#define PCIE_RC_CAL_REG2			0x0020
+#define RC_CAL_TOGGLE			BIT(22)
+#define CLKSEL				GENMASK(31, 29)
+#define CLKSEL_24M		0x3
+
+/* Additional PHY PLL configuration (USB 3 and PCIe) */
+#define PCIE_PU_PLL_1				0x0048
+#define REF_100_WSSC			BIT(12)	/* 1: input is 100MHz, SSC */
+#define FREF_SEL			GENMASK(15, 13)
+#define FREF_24M		0x1
+#define SSC_DEP_SEL			GENMASK(19, 16)
+#define SSC_DEP_NONE		0x0
+#define SSC_DEP_5000PPM		0xa
+
+/* PCIe PHY configuration */
+#define PCIE_PU_PLL_2				0x004c
+#define GEN_REF100			BIT(4)	/* 1: generate 100MHz clk */
+
+#define PCIE_RX_REG1				0x0050
+#define EN_RTERM			BIT(3)
+#define AFE_RTERM_REG			GENMASK(11, 8)
+
+#define PCIE_RX_REG2				0x0054
+#define RX_RTERM_SEL			BIT(5)	/* 0: use AFE_RTERM_REG value */
+
+#define PCIE_LTSSM_DIS_ENTRY			0x005c
+#define CFG_REFCLK_MODE			GENMASK(9, 8)
+#define RFCLK_MODE_DRIVER	0x1
+#define OVRD_REFCLK_MODE		BIT(10)	/* 1: use CFG_RFCLK_MODE */
+
+#define PCIE_TX_REG1				0x0064
+#define TX_RTERM_REG			GENMASK(15, 12)
+#define TX_RTERM_SEL			BIT(25)	/* 1: use TX_RTERM_REG */
+
+/* Zeroed for the combo PHY operating in USB mode */
+#define USB3_TEST_CTRL				0x0068
+
+/* PHY calibration values, determined by the combo PHY at probe time */
+#define PCIE_RCAL_RESULT			0x0084	/* Port A PHY only */
+#define RTERM_VALUE_RX			GENMASK(3, 0)
+#define RTERM_VALUE_TX			GENMASK(7, 4)
+#define R_TUNE_DONE			BIT(10)
+
+static u32 k1_phy_rterm = ~0;     /* Invalid initial value */
+
+/* Save the RX and TX receiver termination values */
+static void k1_phy_rterm_set(u32 val)
+{
+	k1_phy_rterm = val & (RTERM_VALUE_RX | RTERM_VALUE_TX);
+}
+
+static bool k1_phy_rterm_valid(void)
+{
+	/* Valid if no bits outside those we care about are set */
+	return !(k1_phy_rterm & ~(RTERM_VALUE_RX | RTERM_VALUE_TX));
+}
+
+static u32 k1_phy_rterm_rx(void)
+{
+	return FIELD_GET(RTERM_VALUE_RX, k1_phy_rterm);
+}
+
+static u32 k1_phy_rterm_tx(void)
+{
+	return FIELD_GET(RTERM_VALUE_TX, k1_phy_rterm);
+}
+
+/* Only the combo PHY has a PMU pointer defined */
+static bool k1_phy_port_a(struct k1_pcie_phy *k1_phy)
+{
+	return !!k1_phy->pmu;
+}
+
+/* The PLL clocks are driven by the external oscillator */
+static const struct clk_parent_data k1_pcie_phy_data[] = {
+	{ .fw_name = "refclk", },
+};
+
+static struct k1_pcie_phy *clk_hw_to_k1_phy(struct clk_hw *clk_hw)
+{
+	return container_of(clk_hw, struct k1_pcie_phy, pll_hw);
+}
+
+/* USB mode only works on the combo PHY, which has only one lane */
+static void k1_pcie_phy_pll_prepare_usb(struct k1_pcie_phy *k1_phy)
+{
+	void __iomem *regs = k1_phy->regs;
+	u32 val;
+
+	val = readl(regs + PCIE_PU_ADDR_CLK_CFG);
+	val &= ~CFG_INTERNAL_TIMER_ADJ;
+	val |= FIELD_PREP(CFG_INTERNAL_TIMER_ADJ, TIMER_ADJ_USB);
+	writel(val, regs + PCIE_PU_ADDR_CLK_CFG);
+
+	val = readl(regs + PCIE_PU_PLL_1);
+	val &= ~SSC_DEP_SEL;
+	val |= FIELD_PREP(SSC_DEP_SEL, SSC_DEP_5000PPM);
+	writel(val, regs + PCIE_PU_PLL_1);
+}
+
+/* Perform PCIe-specific register updates before starting the PLL clock */
+static void k1_pcie_phy_pll_prepare_pcie(struct k1_pcie_phy *k1_phy)
+{
+	void __iomem *regs = k1_phy->regs;
+	u32 val;
+	u32 i;
+
+	for (i = 0; i < k1_phy->pcie_lanes; i++) {
+		val = readl(regs + PCIE_PU_ADDR_CLK_CFG);
+		val &= ~CFG_INTERNAL_TIMER_ADJ;
+		val |= FIELD_PREP(CFG_INTERNAL_TIMER_ADJ, TIMER_ADJ_PCIE);
+		writel(val, regs + PCIE_PU_ADDR_CLK_CFG);
+
+		regs += PHY_LANE_OFFSET;	/* Next lane */
+	}
+
+	regs = k1_phy->regs;
+	val = readl(regs + PCIE_RC_DONE_STATUS);
+	val |= CFG_FORCE_RCV_RETRY;
+	writel(val, regs + PCIE_RC_DONE_STATUS);
+
+	val = readl(regs + PCIE_PU_PLL_1);
+	val &= ~SSC_DEP_SEL;
+	val |= FIELD_PREP(SSC_DEP_SEL, SSC_DEP_NONE);
+	writel(val, regs + PCIE_PU_PLL_1);
+
+	val = readl(regs + PCIE_PU_PLL_2);
+	val |= GEN_REF100;		/* Enable 100 MHz PLL output clock */
+	writel(val, regs + PCIE_PU_PLL_2);
+}
+
+static int k1_pcie_phy_pll_prepare(struct clk_hw *clk_hw)
+{
+	struct k1_pcie_phy *k1_phy = clk_hw_to_k1_phy(clk_hw);
+	void __iomem *regs = k1_phy->regs;
+	u32 val;
+	u32 i;
+
+	if (k1_phy_port_a(k1_phy) && k1_phy->type == PHY_TYPE_USB3)
+		k1_pcie_phy_pll_prepare_usb(k1_phy);
+	else
+		k1_pcie_phy_pll_prepare_pcie(k1_phy);
+
+	/*
+	 * Disable 100 MHz input reference with spread-spectrum
+	 * clocking and select the 24 MHz clock input frequency
+	 */
+	val = readl(regs + PCIE_PU_PLL_1);
+	val &= ~REF_100_WSSC;
+	val &= ~FREF_SEL;
+	val |= FIELD_PREP(FREF_SEL, FREF_24M);
+	writel(val, regs + PCIE_PU_PLL_1);
+
+	/* Mark PLL configuration done on all lanes */
+	for (i = 0; i < k1_phy->pcie_lanes; i++) {
+		val = readl(regs + PCIE_PU_ADDR_CLK_CFG);
+		val |= CFG_SW_PHY_INIT_DONE;
+		writel(val, regs + PCIE_PU_ADDR_CLK_CFG);
+
+		regs += PHY_LANE_OFFSET;	/* Next lane */
+	}
+
+	/*
+	 * Wait for indication the PHY PLL is locked.  Lanes for ports
+	 * B and C share a PLL, so it's enough to sample just lane 0.
+	 */
+	return readl_poll_timeout(k1_phy->regs + PCIE_PU_ADDR_CLK_CFG,
+				  val, val & PLL_READY,
+				  POLL_DELAY, PLL_TIMEOUT);
+}
+
+/* Prepare implies enable, and once enabled, it's always on */
+static const struct clk_ops k1_pcie_phy_pll_ops = {
+	.prepare	= k1_pcie_phy_pll_prepare,
+};
+
+/* We represent the PHY PLL as a private clock */
+static int k1_pcie_phy_pll_setup(struct k1_pcie_phy *k1_phy)
+{
+	struct clk_hw *hw = &k1_phy->pll_hw;
+	struct device *dev = k1_phy->dev;
+	struct clk_init_data init = { };
+	char *name;
+	int ret;
+
+	name = kasprintf(GFP_KERNEL, "pcie%u_phy_pll", k1_phy->phy->id);
+	if (!name)
+		return -ENOMEM;
+
+	init.name = name;
+	init.ops = &k1_pcie_phy_pll_ops;
+	init.parent_data = k1_pcie_phy_data;
+	init.num_parents = ARRAY_SIZE(k1_pcie_phy_data);
+
+	hw->init = &init;
+
+	ret = devm_clk_hw_register(dev, hw);
+
+	kfree(name);	/* __clk_register() duplicates the name we provide */
+
+	if (ret)
+		return ret;
+
+	k1_phy->pll = devm_clk_hw_get_clk(dev, hw, "pll");
+	if (IS_ERR(k1_phy->pll))
+		return PTR_ERR(k1_phy->pll);
+
+	return 0;
+}
+
+/* Select PCIe or USB 3 mode for the combo PHY. */
+static void k1_combo_phy_sel(struct k1_pcie_phy *k1_phy, bool usb)
+{
+	struct regmap *pmu = k1_phy->pmu;
+
+	/* Only change it if it's not already in the desired state */
+	if (!regmap_test_bits(pmu, PMUA_USB_PHY_CTRL0, COMBO_PHY_SEL) == usb)
+		regmap_assign_bits(pmu, PMUA_USB_PHY_CTRL0, COMBO_PHY_SEL, usb);
+}
+
+static void k1_pcie_phy_init_pcie(struct k1_pcie_phy *k1_phy)
+{
+	u32 rx_rterm = k1_phy_rterm_rx();
+	u32 tx_rterm = k1_phy_rterm_tx();
+	void __iomem *regs;
+	u32 val;
+	int i;
+
+	/* For the combo PHY, set PHY to PCIe mode */
+	if (k1_phy_port_a(k1_phy))
+		k1_combo_phy_sel(k1_phy, false);
+
+	regs = k1_phy->regs;
+	for (i = 0; i < k1_phy->pcie_lanes; i++) {
+		val = readl(regs + PCIE_RX_REG1);
+
+		/* Set RX analog front-end receiver termination value */
+		val &= ~AFE_RTERM_REG;
+		val |= FIELD_PREP(AFE_RTERM_REG, rx_rterm);
+
+		/* And enable refclock receiver termination */
+		val |= EN_RTERM;
+		writel(val, regs + PCIE_RX_REG1);
+
+		val = readl(regs + PCIE_RX_REG2);
+		/* Use PCIE_RX_REG1 AFE_RTERM_REG value */
+		val &= ~RX_RTERM_SEL;
+		writel(val, regs + PCIE_RX_REG2);
+
+		val = readl(regs + PCIE_TX_REG1);
+
+		/* Set TX driver termination value */
+		val &= ~TX_RTERM_REG;
+		val |= FIELD_PREP(TX_RTERM_REG, tx_rterm);
+
+		/* Use PCIE_TX_REG1 TX_RTERM_REG value */
+		val |= TX_RTERM_SEL;
+		writel(val, regs + PCIE_TX_REG1);
+
+		/* Set the input clock to 24 MHz, and clear RC_CAL_TOGGLE */
+		val = readl(regs + PCIE_RC_CAL_REG2);
+		val &= CLKSEL;
+		val |= FIELD_PREP(CLKSEL, CLKSEL_24M);
+		val &= ~RC_CAL_TOGGLE;
+		writel(val, regs + PCIE_RC_CAL_REG2);
+
+		/* Now trigger recalibration by setting RC_CAL_TOGGLE again */
+		val |= RC_CAL_TOGGLE;
+		writel(val, regs + PCIE_RC_CAL_REG2);
+
+		val = readl(regs + PCIE_LTSSM_DIS_ENTRY);
+		/* Override the reference clock; set to refclk driver mode */
+		val |= OVRD_REFCLK_MODE;
+		val &= ~CFG_REFCLK_MODE;
+		val |= FIELD_PREP(CFG_REFCLK_MODE, RFCLK_MODE_DRIVER);
+		writel(val, regs + PCIE_LTSSM_DIS_ENTRY);
+
+		regs += PHY_LANE_OFFSET;	/* Next lane */
+	}
+}
+
+/* Only called for combo PHY */
+static void k1_pcie_phy_init_usb(struct k1_pcie_phy *k1_phy)
+{
+	k1_combo_phy_sel(k1_phy, true);
+
+	/* We're not doing any testing */
+	writel(0, k1_phy->regs + USB3_TEST_CTRL);
+}
+
+static int k1_pcie_phy_init(struct phy *phy)
+{
+	struct k1_pcie_phy *k1_phy = phy_get_drvdata(phy);
+
+	/* Note: port type is only valid for port A (both checks needed) */
+	if (k1_phy_port_a(k1_phy) && k1_phy->type == PHY_TYPE_USB3)
+		k1_pcie_phy_init_usb(k1_phy);
+	else
+		k1_pcie_phy_init_pcie(k1_phy);
+
+
+	return clk_prepare_enable(k1_phy->pll);
+}
+
+static int k1_pcie_phy_exit(struct phy *phy)
+{
+	struct k1_pcie_phy *k1_phy = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(k1_phy->pll);
+
+	return 0;
+}
+
+static const struct phy_ops k1_pcie_phy_ops = {
+	.init		= k1_pcie_phy_init,
+	.exit		= k1_pcie_phy_exit,
+	.owner		= THIS_MODULE,
+};
+
+/*
+ * Get values needed for calibrating PHYs operating in PCIe mode.  Only
+ * the combo PHY is able to do this, and its calibration values are used
+ * for configuring all PCIe PHYs.
+ *
+ * We always need to de-assert the "global" reset on the combo PHY,
+ * because the USB driver depends on it.  If used for PCIe, that driver
+ * will (also) de-assert this, but by leaving it de-asserted for the
+ * combo PHY, the USB driver doesn't have to do this.  Note: although
+ * SpacemiT refers to this as the global reset, we name the "phy" reset.
+ *
+ * In addition, we guarantee the APP_HOLD_PHY_RESET bit is clear for the
+ * combo PHY, so the USB driver doesn't have to manage that either.  The
+ * PCIe driver is free to change this bit for normal operation.
+ *
+ * Calibration only needs to be done once.  It's possible calibration has
+ * already completed (e.g., it might have happened in the boot loader, or
+ * -EPROBE_DEFER might result in this function being called again).  So we
+ * check that early too, to avoid doing it more than once.
+ *
+ * Otherwise we temporarily power up the PHY using the PCIe app clocks
+ * and resets, wait for the hardware to indicate calibration is done,
+ * grab the value, then shut the PHY down again.
+ */
+static int k1_pcie_combo_phy_calibrate(struct k1_pcie_phy *k1_phy)
+{
+	struct reset_control_bulk_data resets[] = {
+		{ .id = "dbi", },
+		{ .id = "mstr", },
+		{ .id = "slv", },
+	};
+	struct clk_bulk_data clocks[] = {
+		{ .id = "dbi", },
+		{ .id = "mstr", },
+		{ .id = "slv", },
+	};
+	struct device *dev = k1_phy->dev;
+	struct reset_control *phy_reset;
+	int ret = 0;
+	int val;
+
+	/* Nothing to do if we already set the receiver termination value */
+	if (k1_phy_rterm_valid())
+		return 0;
+
+	/* De-assert the PHY (global) reset and leave it that way for USB */
+	phy_reset = devm_reset_control_get_exclusive_deasserted(dev, "phy");
+	if (IS_ERR(phy_reset))
+		return PTR_ERR(phy_reset);
+
+	/*
+	 * We also guarantee the APP_HOLD_PHY_RESET bit is clear.  We can
+	 * leave this bit clear even if an error happens below.
+	 */
+	regmap_assign_bits(k1_phy->pmu, PCIE_CLK_RES_CTRL,
+			   PCIE_APP_HOLD_PHY_RST, false);
+
+	/* If the calibration already completed (e.g. by U-Boot), we're done */
+	val = readl(k1_phy->regs + PCIE_RCAL_RESULT);
+	if (val & R_TUNE_DONE)
+		goto out_tune_done;
+
+	/* Put the PHY into PCIe mode */
+	k1_combo_phy_sel(k1_phy, false);
+
+	/* Get and enable the PCIe app clocks */
+	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
+	if (ret <= 0) {
+		if (!ret)
+			ret = -ENOENT;
+		goto out_tune_done;
+	}
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(clocks), clocks);
+	if (ret)
+		goto out_put_clocks;
+
+	/* Get the PCIe application resets (not the PHY reset) */
+	ret = reset_control_bulk_get_shared(dev, ARRAY_SIZE(resets), resets);
+	if (ret)
+		goto out_disable_clocks;
+
+	/* De-assert the PCIe application resets */
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
+	if (ret)
+		goto out_put_resets;
+
+	/*
+	 * This is the core activity here.  Wait for the hardware to
+	 * signal that it has completed calibration/tuning.  Once it
+	 * has, the register value will contain the values we'll
+	 * use to configure PCIe PHYs.
+	 */
+	ret = readl_poll_timeout(k1_phy->regs + PCIE_RCAL_RESULT,
+				 val, val & R_TUNE_DONE,
+				 POLL_DELAY, CALIBRATION_TIMEOUT);
+
+	/* Clean up.  We're done with the resets and clocks */
+	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
+out_put_resets:
+	reset_control_bulk_put(ARRAY_SIZE(resets), resets);
+out_disable_clocks:
+	clk_bulk_disable_unprepare(ARRAY_SIZE(clocks), clocks);
+out_put_clocks:
+	clk_bulk_put_all(ARRAY_SIZE(clocks), clocks);
+out_tune_done:
+	/* If we got the value without timing out, set k1_phy_rterm */
+	if (!ret)
+		k1_phy_rterm_set(val);
+
+	return ret;
+}
+
+static struct phy *
+k1_pcie_combo_phy_xlate(struct device *dev, const struct of_phandle_args *args)
+{
+	struct k1_pcie_phy *k1_phy = dev_get_drvdata(dev);
+	u32 type;
+
+	/* The argument specifying the PHY mode is required */
+	if (args->args_count != 1)
+		return ERR_PTR(-EINVAL);
+
+	/* We only support PCIe and USB 3 mode */
+	type = args->args[0];
+	if (type != PHY_TYPE_PCIE && type != PHY_TYPE_USB3)
+		return ERR_PTR(-EINVAL);
+
+	/* This PHY can only be used once */
+	if (k1_phy->type != PHY_NONE)
+		return ERR_PTR(-EBUSY);
+
+	k1_phy->type = type;
+
+	return k1_phy->phy;
+}
+
+/* Use the maximum number of PCIe lanes unless limited by Device Tree */
+static u32 k1_pcie_num_lanes(struct k1_pcie_phy *k1_phy, bool port_a)
+{
+	struct device *dev = k1_phy->dev;
+	u32 count = 0;
+	u32 max;
+	int ret;
+
+	ret = of_property_read_u32(dev_of_node(dev), "num-lanes", &count);
+	if (count == 1)
+		return 1;
+
+	if (count == 2 && !port_a)
+		return 2;
+
+	max = port_a ? 1 : 2;
+	if (ret != -EINVAL)
+		dev_warn(dev, "bad lane count %u for port; using %u\n",
+			 count, max);
+
+	return max;
+}
+
+static int k1_pcie_combo_phy_probe(struct k1_pcie_phy *k1_phy)
+{
+	struct device *dev = k1_phy->dev;
+	struct regmap *regmap;
+	int ret;
+
+	/* Setting the PHY mode requires access to the PMU regmap */
+	regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev), SYSCON_APMU);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "failed to get PMU\n");
+	k1_phy->pmu = regmap;
+
+	ret = k1_pcie_combo_phy_calibrate(k1_phy);
+	if (ret)
+		return dev_err_probe(dev, ret, "calibration failed\n");
+
+	/* Needed by k1_pcie_combo_phy_xlate(), which also sets k1_phy->type */
+	dev_set_drvdata(dev, k1_phy);
+
+	return 0;
+}
+
+static int k1_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct phy *(*xlate)(struct device *dev,
+			     const struct of_phandle_args *args);
+	struct device *dev = &pdev->dev;
+	struct phy_provider *provider;
+	struct k1_pcie_phy *k1_phy;
+	bool probing_port_a;
+	int ret;
+
+	xlate = of_device_get_match_data(dev);
+	probing_port_a = xlate == k1_pcie_combo_phy_xlate;
+
+	/* Only the combo PHY can calibrate, so it must probe first */
+	if (!k1_phy_rterm_valid() && !probing_port_a)
+		return -EPROBE_DEFER;
+
+	k1_phy = devm_kzalloc(dev, sizeof(*k1_phy), GFP_KERNEL);
+	if (!k1_phy)
+		return -ENOMEM;
+	k1_phy->dev = dev;
+
+	k1_phy->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(k1_phy->regs))
+		return dev_err_probe(dev, PTR_ERR(k1_phy->regs),
+				     "error mapping registers\n");
+
+	if (probing_port_a) {
+		ret = k1_pcie_combo_phy_probe(k1_phy);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "error probing combo phy\n");
+	}
+
+	k1_phy->pcie_lanes = k1_pcie_num_lanes(k1_phy, probing_port_a);
+
+	k1_phy->phy = devm_phy_create(dev, NULL, &k1_pcie_phy_ops);
+	if (IS_ERR(k1_phy->phy))
+		return dev_err_probe(dev, PTR_ERR(k1_phy->phy),
+				     "error creating phy\n");
+	phy_set_drvdata(k1_phy->phy, k1_phy);
+
+	ret = k1_pcie_phy_pll_setup(k1_phy);
+	if (ret)
+		return dev_err_probe(dev, ret, "error initializing clock\n");
+
+	provider = devm_of_phy_provider_register(dev, xlate);
+	if (IS_ERR(provider))
+		return dev_err_probe(dev, PTR_ERR(provider),
+				     "error registering provider\n");
+	return 0;
+}
+
+static const struct of_device_id k1_pcie_phy_of_match[] = {
+	{ .compatible = "spacemit,k1-combo-phy", k1_pcie_combo_phy_xlate, },
+	{ .compatible = "spacemit,k1-pcie-phy", of_phy_simple_xlate, },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, k1_pcie_phy_of_match);
+
+static struct platform_driver k1_pcie_phy_driver = {
+	.probe	= k1_pcie_phy_probe,
+	.driver = {
+		.of_match_table	= k1_pcie_phy_of_match,
+		.name = "spacemit-k1-pcie-phy",
+	}
+};
+module_platform_driver(k1_pcie_phy_driver);
+
+MODULE_DESCRIPTION("SpacemiT K1 PCIe and USB 3 PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.48.1

[PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Alex Elder]

Introduce a driver for the PCIe host controller found in the SpacemiT
K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
The driver supports three PCIe ports that operate at PCIe gen2 transfer
rates (5 GT/sec).  The first port uses a combo PHY, which may be
configured for use for USB 3 instead.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Renamed the PCIe driver source file "pcie-spacemit-k1.c"
    - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1; it
      is now tristate rather than Boolean
    - The PCIe host compatible string is now "spacemit,k1-pcie"
    - Renamed the PMU syscon property to be "spacemit,apmu"
    - Renamed the symbols representing the PCI vendor and device IDs
      to align with <linux/pci_ids.h>
    - Use PCIE_T_PVPERL_MS rather than 100 to represent a standard
      delay period.
    - Use platform (not dev) driver-data access functions; assignment
      is done only after the private structure is initialized
    - Deleted some unneeded includes in the PCIe driver.
    - Dropped error checking when operating on MMIO-backed regmaps
    - Added a regmap_read() call in two places, to ensure a specified
      delay occurs *after* the a MMIO write has reached its target.
    - Used ARRAY_SIZE() (not a local variable value) in a few spots
    - Now use readl_relaxed()/writel_relaxed() when operating on
      the "link" I/O memory space in the PCIe driver
    - Updated a few error messages for consistency
    - No longer specify suppress_bind_attrs in the PCIe driver
    - Now specify PCIe driver probe type as PROBE_PREFER_ASYNCHRONOUS
    - No longer use (void) cast to indicate ignored return values

 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 319 ++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-spacemit-k1.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 349d4657393c9..ede59b34c99ba 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -509,6 +509,16 @@ config PCI_KEYSTONE_EP
 	  on DesignWare hardware and therefore the driver re-uses the
 	  DesignWare core functions to implement the driver.
 
+config PCIE_SPACEMIT_K1
+	tristate "SpacemiT K1 PCIe controller (host mode)"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on PCI && OF && HAS_IOMEM
+	select PCIE_DW_HOST
+	default ARCH_SPACEMIT
+	help
+	  Enables support for the PCIe controller in the K1 SoC operating
+	  in host mode.
+
 config PCIE_VISCONTI_HOST
 	bool "Toshiba Visconti PCIe controller"
 	depends on ARCH_VISCONTI || COMPILE_TEST
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 7ae28f3b0fb39..662b0a219ddc4 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
+obj-$(CONFIG_PCIE_SPACEMIT_K1) += pcie-spacemit-k1.o
 obj-$(CONFIG_PCIE_STM32_HOST) += pcie-stm32.o
 obj-$(CONFIG_PCIE_STM32_EP) += pcie-stm32-ep.o
 
diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
new file mode 100644
index 0000000000000..d58232cbb8a02
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SpacemiT K1 PCIe host driver
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ * Copyright (c) 2023, spacemit Corporation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define PCI_VENDOR_ID_SPACEMIT		0x201f
+#define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
+
+/* Offsets and field definitions for link management registers */
+
+#define K1_PHY_AHB_IRQ_EN			0x0000
+#define PCIE_INTERRUPT_EN		BIT(0)
+
+#define K1_PHY_AHB_LINK_STS			0x0004
+#define SMLH_LINK_UP			BIT(1)
+#define RDLH_LINK_UP			BIT(12)
+
+#define INTR_ENABLE				0x0014
+#define MSI_CTRL_INT			BIT(11)
+
+/* Some controls require APMU regmap access */
+#define SYSCON_APMU			"spacemit,apmu"
+
+/* Offsets and field definitions for APMU registers */
+
+#define PCIE_CLK_RESET_CONTROL			0x0000
+#define LTSSM_EN			BIT(6)
+#define PCIE_AUX_PWR_DET		BIT(9)
+#define PCIE_RC_PERST			BIT(12)	/* 1: assert PERST# */
+#define APP_HOLD_PHY_RST		BIT(30)
+#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
+
+#define PCIE_CONTROL_LOGIC			0x0004
+#define PCIE_SOFT_RESET			BIT(0)
+
+struct k1_pcie {
+	struct dw_pcie pci;
+	struct phy *phy;
+	void __iomem *link;
+	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
+	u32 pmu_off;
+};
+
+#define to_k1_pcie(dw_pcie) \
+		platform_get_drvdata(to_platform_device((dw_pcie)->dev))
+
+static void k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
+{
+	u32 offset;
+	u32 val;
+
+	/*
+	 * Write, then read back to guarantee it has reached the device
+	 * before we start the delay.
+	 */
+	offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
+	regmap_set_bits(k1->pmu, offset, PCIE_SOFT_RESET);
+	regmap_read(k1->pmu, offset, &val);
+
+	mdelay(2);
+
+	regmap_clear_bits(k1->pmu, offset, PCIE_SOFT_RESET);
+}
+
+/* Enable app clocks, deassert resets */
+static int k1_pcie_activate(struct k1_pcie *k1)
+{
+	struct dw_pcie *pci = &k1->pci;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(pci->app_clks), pci->app_clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->app_rsts),
+					  pci->app_rsts);
+	if (ret)
+		goto err_disable_clks;
+
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->core_rsts),
+					  pci->core_rsts);
+	if (ret)
+		goto err_assert_resets;
+
+	return 0;
+
+err_assert_resets:
+	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
+err_disable_clks:
+	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
+
+	return ret;
+}
+
+/* Assert resets, disable app clocks */
+static void k1_pcie_deactivate(struct k1_pcie *k1)
+{
+	struct dw_pcie *pci = &k1->pci;
+
+	reset_control_bulk_assert(ARRAY_SIZE(pci->core_rsts), pci->core_rsts);
+	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
+}
+
+static int k1_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 offset;
+	u32 mask;
+	u32 val;
+	int ret;
+
+	k1_pcie_toggle_soft_reset(k1);
+
+	ret = k1_pcie_activate(k1);
+	if (ret)
+		return ret;
+
+	ret = phy_init(k1->phy);
+	if (ret) {
+		k1_pcie_deactivate(k1);
+
+		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);
+
+	/*
+	 * Assert fundamental reset (drive PERST# low).  Put the port in
+	 * root complex mode, and indicate that Vaux (3.3v) is present.
+	 */
+	mask = PCIE_RC_PERST;
+	mask |= DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
+
+	/*
+	 * Write, then read back to guarantee it has reached the device
+	 * before we start the delay.
+	 */
+	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
+	regmap_set_bits(k1->pmu, offset, mask);
+	regmap_read(k1->pmu, offset, &val);
+
+	mdelay(PCIE_T_PVPERL_MS);
+
+	/* Deassert fundamental reset (drive PERST# high) */
+	regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
+
+	return 0;
+}
+
+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);
+
+	/* 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);
+
+	k1_pcie_deactivate(k1);
+}
+
+static const struct dw_pcie_host_ops k1_pcie_host_ops = {
+	.init		= k1_pcie_init,
+	.deinit		= k1_pcie_deinit,
+};
+
+static bool k1_pcie_link_up(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 val;
+
+	val = readl_relaxed(k1->link + K1_PHY_AHB_LINK_STS);
+
+	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
+}
+
+static int k1_pcie_start_link(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 val;
+
+	/* Stop holding the PHY in reset, and enable link training */
+	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+			   APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
+
+	/* Enable the MSI interrupt */
+	writel_relaxed(MSI_CTRL_INT, k1->link + INTR_ENABLE);
+
+	/* Top-level interrupt enable */
+	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
+	val |= PCIE_INTERRUPT_EN;
+	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
+
+	return 0;
+}
+
+static void k1_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 val;
+
+	/* Disable interrupts */
+	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
+	val &= ~PCIE_INTERRUPT_EN;
+	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
+
+	writel_relaxed(0, k1->link + INTR_ENABLE);
+
+	/* Disable the link and hold the PHY in reset */
+	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+			   APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
+}
+
+static const struct dw_pcie_ops k1_pcie_ops = {
+	.link_up	= k1_pcie_link_up,
+	.start_link	= k1_pcie_start_link,
+	.stop_link	= k1_pcie_stop_link,
+};
+
+static int k1_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct k1_pcie *k1;
+	int ret;
+
+	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
+	if (!k1)
+		return -ENOMEM;
+
+	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
+						       SYSCON_APMU, 1,
+						       &k1->pmu_off);
+	if (IS_ERR(k1->pmu))
+		return dev_err_probe(dev, PTR_ERR(k1->pmu),
+				     "failed to lookup PMU registers\n");
+
+	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
+	if (!k1->link)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to map \"link\" registers\n");
+
+	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get \"vpcie3v3\" supply\n");
+
+	/* Hold the PHY in reset until we start the link */
+	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+			APP_HOLD_PHY_RST);
+
+	k1->phy = devm_phy_get(dev, NULL);
+	if (IS_ERR(k1->phy))
+		return dev_err_probe(dev, PTR_ERR(k1->phy),
+				     "failed to get PHY\n");
+
+	k1->pci.dev = dev;
+	k1->pci.ops = &k1_pcie_ops;
+	dw_pcie_cap_set(&k1->pci, REQ_RES);
+
+	k1->pci.pp.ops = &k1_pcie_host_ops;
+	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
+
+	platform_set_drvdata(pdev, k1);
+
+	ret = dw_pcie_host_init(&k1->pci.pp);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize host\n");
+
+	return 0;
+}
+
+static void k1_pcie_remove(struct platform_device *pdev)
+{
+	struct k1_pcie *k1 = platform_get_drvdata(pdev);
+
+	dw_pcie_host_deinit(&k1->pci.pp);
+}
+
+static const struct of_device_id k1_pcie_of_match_table[] = {
+	{ .compatible = "spacemit,k1-pcie", },
+	{ },
+};
+
+static struct platform_driver k1_pcie_driver = {
+	.probe	= k1_pcie_probe,
+	.remove	= k1_pcie_remove,
+	.driver = {
+		.name			= "spacemit-k1-pcie",
+		.of_match_table		= k1_pcie_of_match_table,
+		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+module_platform_driver(k1_pcie_driver);
-- 
2.48.1

[PATCH v2 6/7] riscv: dts: spacemit: add a PCIe regulator

[Alex Elder]

Define a 3.3v fixed voltage regulator to be used by PCIe on the
Banana Pi BPI-F3.  On this platform, this regulator is always on.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - New patch, for a newly-added regulator

 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 2aaaff77831e1..046ad441b7b4e 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -30,6 +30,14 @@ led1 {
 			default-state = "on";
 		};
 	};
+
+	pcie_vcc_3v3: pcie-vcc3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "PCIE_VCC3V3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-always-on;
+	};
 };
 
 &emmc {
-- 
2.48.1

[PATCH v2 7/7] riscv: dts: spacemit: PCIe and PHY-related updates

[Alex Elder]

Define PCIe and PHY-related Device Tree nodes for the SpacemiT K1 SoC.

Enable the combo PHY and the two PCIe-only PHYs on the Banana Pi BPI-F3
board.  The combo PHY is used for USB on this board, and that will be
enabled when USB 3 support is accepted.

The combo PHY must perform a calibration step to determine configuration
values used by the PCIe-only PHYs.  As a result, it must be enabled if
either of the other two PHYs is enabled.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: - Added vpcie3v3-supply nodes to PCIe ports
    - Combo PHY node is now defined earlier in the file (alphabetized)

 .../boot/dts/spacemit/k1-bananapi-f3.dts      |  30 ++++
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  |  33 ++++
 arch/riscv/boot/dts/spacemit/k1.dtsi          | 151 ++++++++++++++++++
 3 files changed, 214 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 046ad441b7b4e..6d566780aed9d 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -40,6 +40,12 @@ pcie_vcc_3v3: pcie-vcc3v3 {
 	};
 };
 
+&combo_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie0_3_cfg>;
+	status = "okay";
+};
+
 &emmc {
 	bus-width = <8>;
 	mmc-hs400-1_8v;
@@ -100,6 +106,30 @@ &pdma {
 	status = "okay";
 };
 
+&pcie1_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie1_3_cfg>;
+	status = "okay";
+};
+
+&pcie2_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie2_4_cfg>;
+	status = "okay";
+};
+
+&pcie1 {
+	phys = <&pcie1_phy>;
+	vpcie3v3-supply = <&pcie_vcc_3v3>;
+	status = "okay";
+};
+
+&pcie2 {
+	phys = <&pcie2_phy>;
+	vpcie3v3-supply = <&pcie_vcc_3v3>;
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_2_cfg>;
diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
index aff19c86d5ff3..5bacb6aff23f8 100644
--- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
@@ -69,6 +69,39 @@ uart0-2-pins {
 		};
 	};
 
+	pcie0_3_cfg: pcie0-3-cfg {
+		pcie0-3-pins {
+			pinmux = <K1_PADCONF(54, 3)>,	/* PERST# */
+				 <K1_PADCONF(55, 3)>,	/* WAKE# */
+				 <K1_PADCONF(53, 3)>;	/* CLKREQ# */
+
+			bias-pull-up = <0>;
+			drive-strength = <21>;
+		};
+	};
+
+	pcie1_3_cfg: pcie1-3-cfg {
+		pcie1-3-pins {
+			pinmux = <K1_PADCONF(59, 4)>,	/* PERST# */
+				 <K1_PADCONF(60, 4)>,	/* WAKE# */
+				 <K1_PADCONF(61, 4)>;	/* CLKREQ# */
+
+			bias-pull-up = <0>;
+			drive-strength = <21>;
+		};
+	};
+
+	pcie2_4_cfg: pcie2-4-cfg {
+		pcie2-4-pins {
+			pinmux = <K1_PADCONF(62, 4)>,	/* PERST# */
+				 <K1_PADCONF(112, 3)>,	/* WAKE# */
+				 <K1_PADCONF(117, 4)>;	/* CLKREQ# */
+
+			bias-pull-up = <0>;
+			drive-strength = <21>;
+		};
+	};
+
 	pwm14_1_cfg: pwm14-1-cfg {
 		pwm14-1-pins {
 			pinmux = <K1_PADCONF(44, 4)>;
diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 6cdcd80a7c83b..a38c578f24004 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -4,6 +4,7 @@
  */
 
 #include <dt-bindings/clock/spacemit,k1-syscon.h>
+#include <dt-bindings/phy/phy.h>
 
 /dts-v1/;
 / {
@@ -358,6 +359,48 @@ syscon_rcpu2: system-controller@c0888000 {
 			#reset-cells = <1>;
 		};
 
+		combo_phy: phy@c0b10000 {
+			compatible = "spacemit,k1-combo-phy";
+			reg = <0x0 0xc0b10000 0x0 0x1000>;
+			clocks = <&vctcxo_24m>,
+				 <&syscon_apmu CLK_PCIE0_DBI>,
+				 <&syscon_apmu CLK_PCIE0_MASTER>,
+				 <&syscon_apmu CLK_PCIE0_SLAVE>;
+			clock-names = "refclk",
+				      "dbi",
+				      "mstr",
+				      "slv";
+			resets = <&syscon_apmu RESET_PCIE0_DBI>,
+				 <&syscon_apmu RESET_PCIE0_MASTER>,
+				 <&syscon_apmu RESET_PCIE0_SLAVE>,
+				 <&syscon_apmu RESET_PCIE0_GLOBAL>;
+			reset-names = "dbi",
+				      "mstr",
+				      "slv",
+				      "phy";
+			#phy-cells = <1>;
+			spacemit,apmu = <&syscon_apmu>;
+			status = "disabled";
+		};
+
+		pcie1_phy: phy@c0c10000 {
+			compatible = "spacemit,k1-pcie-phy";
+			reg = <0x0 0xc0c10000 0x0 0x1000>;
+			clocks = <&vctcxo_24m>;
+			clock-names = "refclk";
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
+		pcie2_phy: phy@c0d10000 {
+			compatible = "spacemit,k1-pcie-phy";
+			clocks = <&vctcxo_24m>;
+			clock-names = "refclk";
+			reg = <0x0 0xc0d10000 0x0 0x1000>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
 		syscon_apbc: system-controller@d4015000 {
 			compatible = "spacemit,k1-syscon-apbc";
 			reg = <0x0 0xd4015000 0x0 0x1000>;
@@ -847,6 +890,114 @@ pcie-bus {
 			#size-cells = <2>;
 			dma-ranges = <0x0 0x00000000 0x0 0x00000000 0x0 0x80000000>,
 				     <0x0 0xb8000000 0x1 0x38000000 0x3 0x48000000>;
+			pcie0: pcie@ca000000 {
+				compatible = "spacemit,k1-pcie";
+				reg = <0x0 0xca000000 0x0 0x00001000>,
+				      <0x0 0xca300000 0x0 0x0001ff24>,
+				      <0x0 0x8f000000 0x0 0x00002000>,
+				      <0x0 0xc0b20000 0x0 0x00001000>;
+				reg-names = "dbi",
+					    "atu",
+					    "config",
+					    "link";
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges = <0x01000000 0x0 0x00000000 0x0 0x8f002000 0x0 0x00100000>,
+					 <0x02000000 0x0 0x80000000 0x0 0x80000000 0x0 0x0f000000>;
+				interrupts = <141>;
+				interrupt-names = "msi";
+				clocks = <&syscon_apmu CLK_PCIE0_DBI>,
+					 <&syscon_apmu CLK_PCIE0_MASTER>,
+					 <&syscon_apmu CLK_PCIE0_SLAVE>;
+				clock-names = "dbi",
+					      "mstr",
+					      "slv";
+				resets = <&syscon_apmu RESET_PCIE0_DBI>,
+					 <&syscon_apmu RESET_PCIE0_MASTER>,
+					 <&syscon_apmu RESET_PCIE0_SLAVE>,
+					 <&syscon_apmu RESET_PCIE0_GLOBAL>;
+				reset-names = "dbi",
+					      "mstr",
+					      "slv",
+					      "phy";
+				device_type = "pci";
+				num-viewport = <8>;
+				spacemit,apmu = <&syscon_apmu 0x03cc>;
+				status = "disabled";
+			};
+
+			pcie1: pcie@ca400000 {
+				compatible = "spacemit,k1-pcie";
+				reg = <0x0 0xca400000 0x0 0x00001000>,
+				      <0x0 0xca700000 0x0 0x0001ff24>,
+				      <0x0 0x9f000000 0x0 0x00002000>,
+				      <0x0 0xc0c20000 0x0 0x00001000>;
+				reg-names = "dbi",
+					    "atu",
+					    "config",
+					    "link";
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges = <0x01000000 0x0 0x00000000 0x0 0x9f002000 0x0 0x00100000>,
+					 <0x02000000 0x0 0x90000000 0x0 0x90000000 0x0 0x0f000000>;
+				interrupts = <142>;
+				interrupt-names = "msi";
+				clocks = <&syscon_apmu CLK_PCIE1_DBI>,
+					 <&syscon_apmu CLK_PCIE1_MASTER>,
+					 <&syscon_apmu CLK_PCIE1_SLAVE>;
+				clock-names = "dbi",
+					      "mstr",
+					      "slv";
+				resets = <&syscon_apmu RESET_PCIE1_DBI>,
+					 <&syscon_apmu RESET_PCIE1_MASTER>,
+					 <&syscon_apmu RESET_PCIE1_SLAVE>,
+					 <&syscon_apmu RESET_PCIE1_GLOBAL>;
+				reset-names = "dbi",
+					      "mstr",
+					      "slv",
+					      "phy";
+				device_type = "pci";
+				num-viewport = <8>;
+				spacemit,apmu = <&syscon_apmu 0x3d4>;
+				status = "disabled";
+			};
+
+			pcie2: pcie@ca800000 {
+				compatible = "spacemit,k1-pcie";
+				reg = <0x0 0xca800000 0x0 0x00001000>,
+				      <0x0 0xcab00000 0x0 0x0001ff24>,
+				      <0x0 0xb7000000 0x0 0x00002000>,
+				      <0x0 0xc0d20000 0x0 0x00001000>;
+				reg-names = "dbi",
+					    "atu",
+					    "config",
+					    "link";
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges = <0x01000000 0x0 0x00000000 0x0 0xb7002000 0x0 0x00100000>,
+					 <0x42000000 0x0 0xa0000000 0x0 0xa0000000 0x0 0x10000000>,
+					 <0x02000000 0x0 0xb0000000 0x0 0xb0000000 0x0 0x07000000>;
+				interrupts = <143>;
+				interrupt-names = "msi";
+				clocks = <&syscon_apmu CLK_PCIE2_DBI>,
+					 <&syscon_apmu CLK_PCIE2_MASTER>,
+					 <&syscon_apmu CLK_PCIE2_SLAVE>;
+				clock-names = "dbi",
+					      "mstr",
+					      "slv";
+				resets = <&syscon_apmu RESET_PCIE2_DBI>,
+					 <&syscon_apmu RESET_PCIE2_MASTER>,
+					 <&syscon_apmu RESET_PCIE2_SLAVE>,
+					 <&syscon_apmu RESET_PCIE2_GLOBAL>;
+				reset-names = "dbi",
+					      "mstr",
+					      "slv",
+					      "phy";
+				device_type = "pci";
+				num-viewport = <8>;
+				spacemit,apmu = <&syscon_apmu 0x3dc>;
+				status = "disabled";
+			};
 		};
 
 		storage-bus {
-- 
2.48.1

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Yao Zi]

On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
> Add the Device Tree binding for the PCIe root complex found on the
> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
> typically used to support a USB 3 port.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Renamed the binding, using "host controller"
>     - Added '>' to the description, and reworded it a bit
>     - Added reference to /schemas/pci/snps,dw-pcie.yaml
>     - Fixed and renamed the compatible string
>     - Renamed the PMU property, and fixed its description
>     - Consistently omit the period at the end of descriptions
>     - Renamed the "global" clock to be "phy"
>     - Use interrupts rather than interrupts-extended, and name the
>       one interrupt "msi" to make clear its purpose
>     - Added a vpcie3v3-supply property
>     - Dropped the max-link-speed property
>     - Changed additionalProperties to unevaluatedProperties
>     - Dropped the label and status property from the example
> 
>  .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> new file mode 100644
> index 0000000000000..87745d49c53a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 PCI Express Host Controller
> +
> +maintainers:
> +  - Alex Elder <elder@riscstar.com>
> +
> +description: >
> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
> +  DesignWare 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:

...

> +  num-viewport:
> +    const: 8

This property has been deprecated for a long time, and the driver now
detects viewports at runtime since commit 281f1f99cf3a (PCI: dwc: Detect
number of iATU windows, 2020-11-05), IOW, it makes no effect with the
current mainline DWC PCIe driver. Is it really necessary?

Best regards,
Yao Zi

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Alex Elder]

On 10/13/25 8:55 PM, Yao Zi wrote:
> On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
>> Add the Device Tree binding for the PCIe root complex found on the
>> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
>> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
>> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
>> typically used to support a USB 3 port.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - Renamed the binding, using "host controller"
>>      - Added '>' to the description, and reworded it a bit
>>      - Added reference to /schemas/pci/snps,dw-pcie.yaml
>>      - Fixed and renamed the compatible string
>>      - Renamed the PMU property, and fixed its description
>>      - Consistently omit the period at the end of descriptions
>>      - Renamed the "global" clock to be "phy"
>>      - Use interrupts rather than interrupts-extended, and name the
>>        one interrupt "msi" to make clear its purpose
>>      - Added a vpcie3v3-supply property
>>      - Dropped the max-link-speed property
>>      - Changed additionalProperties to unevaluatedProperties
>>      - Dropped the label and status property from the example
>>
>>   .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>>   1 file changed, 156 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> new file mode 100644
>> index 0000000000000..87745d49c53a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> @@ -0,0 +1,156 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: SpacemiT K1 PCI Express Host Controller
>> +
>> +maintainers:
>> +  - Alex Elder <elder@riscstar.com>
>> +
>> +description: >
>> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
>> +  DesignWare 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:
> 
> ...
> 
>> +  num-viewport:
>> +    const: 8
> 
> This property has been deprecated for a long time, and the driver now
> detects viewports at runtime since commit 281f1f99cf3a (PCI: dwc: Detect
> number of iATU windows, 2020-11-05), IOW, it makes no effect with the
> current mainline DWC PCIe driver. Is it really necessary?

Based on what you say, the answer is "no" and I'll gladly remove it.

Thanks.

					-Alex

> Best regards,
> Yao Zi

Re: [PATCH v2 1/7] dt-bindings: phy: spacemit: add SpacemiT PCIe/combo PHY

[Rob Herring]

On Mon, Oct 13, 2025 at 10:35:18AM -0500, Alex Elder wrote:
> Add the Device Tree binding for the PCIe/USB 3.0 combo PHY found in
> the SpacemiT K1 SoC.  This is one of three PCIe PHYs, and is unusual
> in that only the combo PHY can perform a calibration step needed to
> determine settings used by the other two PCIe PHYs.
> 
> Calibration must be done with the combo PHY in PCIe mode, and to allow
> this to occur independent of the eventual use for the PHY (PCIe or USB)
> some PCIe-related properties must be supplied: clocks; resets; and a
> syscon phandle.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Added '>' to the description, and reworded it a bit
>     - Added an external oscillator clock, "refclk"
>     - Renamed the "global" reset to be "phy"
>     - Renamed a phandle property to be "spacemit,apmu"
>     - Dropped the label and status property from the example
> 
>  .../bindings/phy/spacemit,k1-combo-phy.yaml   | 114 ++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml b/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
> new file mode 100644
> index 0000000000000..6e2f401b0ac27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/spacemit,k1-combo-phy.yaml
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/spacemit,k1-combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 PCIe/USB3 Combo PHY
> +
> +maintainers:
> +  - Alex Elder <elder@riscstar.com>
> +
> +description: >
> +  Of the three PHYs on the SpacemiT K1 SoC capable of being used for
> +  PCIe, one is a combo PHY that can also be configured for use by a
> +  USB 3 controller.  Using PCIe or USB 3 is a board design decision.
> +
> +  The combo PHY is also the only PCIe PHY that is able to determine
> +  PCIe calibration values to use, and this must be determined before
> +  the other two PCIe PHYs can be used.  This calibration must be
> +  performed with the combo PHY in PCIe mode, and is this is done
> +  when the combo PHY is probed.
> +
> +  The combo PHY uses an external oscillator as a reference clock.
> +  During normal operation, the PCIe or USB port driver is responsible
> +  for ensuring all other clocks needed by a PHY are enabled, and all
> +  resets affecting the PHY are deasserted.  However, for the combo
> +  PHY to perform calibration independent of whether it's later used
> +  for PCIe or USB, all PCIe mode clocks and resets must be defined.
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-combo-phy
> +
> +  reg:
> +    items:
> +      - description: PHY control registers
> +
> +  clocks:
> +    items:
> +      - description: External oscillator used by the PHY PLL
> +      - 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: refclk
> +      - 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
> +      - description: PHY reset; must be deasserted for PHY to function
> +
> +  reset-names:
> +    items:
> +      - const: dbi
> +      - const: mstr
> +      - const: slv
> +      - const: phy

I think phy should be first as that's the main one to the phy and the 
others are somewhat questionable. Otherwise,

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 2/7] dt-bindings: phy: spacemit: introduce PCIe PHY

[Rob Herring (Arm)]

On Mon, 13 Oct 2025 10:35:19 -0500, Alex Elder wrote:
> Add the Device Tree binding for two PCIe PHYs present on the SpacemiT
> K1 SoC.  These PHYs are dependent on a separate combo PHY, which
> determines at probe time the calibration values used by the PCIe-only
> PHYs.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Added '>' to the description, and reworded it a bit
>     - Added clocks and clock-names properties
>     - Dropped the label and status property from the example
> 
>  .../bindings/phy/spacemit,k1-pcie-phy.yaml    | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Rob Herring]

On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
> Add the Device Tree binding for the PCIe root complex found on the
> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
> typically used to support a USB 3 port.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Renamed the binding, using "host controller"
>     - Added '>' to the description, and reworded it a bit
>     - Added reference to /schemas/pci/snps,dw-pcie.yaml
>     - Fixed and renamed the compatible string
>     - Renamed the PMU property, and fixed its description
>     - Consistently omit the period at the end of descriptions
>     - Renamed the "global" clock to be "phy"
>     - Use interrupts rather than interrupts-extended, and name the
>       one interrupt "msi" to make clear its purpose
>     - Added a vpcie3v3-supply property
>     - Dropped the max-link-speed property
>     - Changed additionalProperties to unevaluatedProperties
>     - Dropped the label and status property from the example
> 
>  .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> new file mode 100644
> index 0000000000000..87745d49c53a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 PCI Express Host Controller
> +
> +maintainers:
> +  - Alex Elder <elder@riscstar.com>
> +
> +description: >
> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
> +  DesignWare 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
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: ATU address space
> +      - description: PCIe configuration space
> +      - description: Link control registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: atu
> +      - const: config
> +      - const: link
> +
> +  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
> +
> +  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
> +      - description: Global reset; must be deasserted for PHY to function
> +
> +  reset-names:
> +    items:
> +      - const: dbi
> +      - const: mstr
> +      - const: slv
> +      - const: phy

You expect/need the phy driver and PCIe driver to both reset the PHY? 
You should do that indirectly with the PHY API when you reset the 
controller.

Rob

Re: [PATCH v2 4/7] phy: spacemit: introduce PCIe/combo PHY

[Aurelien Jarno]

Hi,

On 2025-10-13 10:35, Alex Elder wrote:
> Introduce a driver that supports three PHYs found on the SpacemiT
> K1 SoC.  The first PHY is a combo PHY that can be configured for
> use for either USB 3 or PCIe.  The other two PHYs support PCIe
> only.
> 
> All three PHYs must be programmed with an 8 bit receiver termination
> value, which must be determined dynamically.  Only the combo PHY is
> able to determine this value.  The combo PHY performs a special
> calibration step at probe time to discover this, and that value is
> used to program each PHY that operates in PCIe mode.  The combo
> PHY must therefore be probed before either of the PCIe-only PHYs
> will be used.
> 
> Each PHY has an internal PLL driven from an external oscillator.
> This PLL started when the PHY is first initialized, and stays
> on thereafter.
> 
> During normal operation, the USB or PCIe driver using the PHY must
> ensure (other) clocks and resets are set up properly.
> 
> However PCIe mode clocks are enabled and resets are de-asserted
> temporarily by this driver to perform the calibration step on the
> combo PHY.
> 
> Tested-by: Junzhong Pan <panjunzhong@linux.spacemit.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>

Thanks for this new version. I have tried it on top of v6.18-rc1 + 
spacemit DTS commits from next on a BPI-F3, and it fails calibrating the 
PHY with:

[    2.748405] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: calibration failed
[    2.755300] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: error probing combo phy
[    2.763088] spacemit-k1-pcie-phy c0b10000.phy: probe with driver spacemit-k1-pcie-phy failed with error -2
[   14.309031] platform c0d10000.phy: deferred probe pending: (reason unknown)
[   14.313426] platform c0c10000.phy: deferred probe pending: (reason unknown)
[   14.320347] platform ca400000.pcie: deferred probe pending: platform: supplier c0c10000.phy not ready
[   14.329542] platform ca800000.pcie: deferred probe pending: platform: supplier c0d10000.phy not ready

Note that version 1 was working fine on the same board.

[ snip ]

> diff --git a/drivers/phy/phy-spacemit-k1-pcie.c b/drivers/phy/phy-spacemit-k1-pcie.c
> new file mode 100644
> index 0000000000000..81bc05823d080
> --- /dev/null
> +++ b/drivers/phy/phy-spacemit-k1-pcie.c

[ snip ]

> +static int k1_pcie_combo_phy_calibrate(struct k1_pcie_phy *k1_phy)
> +{
> +	struct reset_control_bulk_data resets[] = {
> +		{ .id = "dbi", },
> +		{ .id = "mstr", },
> +		{ .id = "slv", },
> +	};
> +	struct clk_bulk_data clocks[] = {
> +		{ .id = "dbi", },
> +		{ .id = "mstr", },
> +		{ .id = "slv", },
> +	};
> +	struct device *dev = k1_phy->dev;
> +	struct reset_control *phy_reset;
> +	int ret = 0;
> +	int val;
> +
> +	/* Nothing to do if we already set the receiver termination value */
> +	if (k1_phy_rterm_valid())
> +		return 0;
> +
> +	/* De-assert the PHY (global) reset and leave it that way for USB */
> +	phy_reset = devm_reset_control_get_exclusive_deasserted(dev, "phy");
> +	if (IS_ERR(phy_reset))
> +		return PTR_ERR(phy_reset);
> +
> +	/*
> +	 * We also guarantee the APP_HOLD_PHY_RESET bit is clear.  We can
> +	 * leave this bit clear even if an error happens below.
> +	 */
> +	regmap_assign_bits(k1_phy->pmu, PCIE_CLK_RES_CTRL,
> +			   PCIE_APP_HOLD_PHY_RST, false);
> +
> +	/* If the calibration already completed (e.g. by U-Boot), we're done */
> +	val = readl(k1_phy->regs + PCIE_RCAL_RESULT);
> +	if (val & R_TUNE_DONE)
> +		goto out_tune_done;
> +
> +	/* Put the PHY into PCIe mode */
> +	k1_combo_phy_sel(k1_phy, false);
> +
> +	/* Get and enable the PCIe app clocks */
> +	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -ENOENT;
> +		goto out_tune_done;
> +	}

This part doesn't look correct. The documentation says this function 
"returns 0 if all clocks specified in clk_bulk_data table are obtained
successfully, or valid IS_ERR() condition containing errno."

To me, it seems the code should only be:

	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
	if (ret)
		goto out_tune_done;

[snip]

> +out_put_clocks:
> +	clk_bulk_put_all(ARRAY_SIZE(clocks), clocks);

When fixing the above bug, this then crashes with:

[    2.776109] Unable to handle kernel paging request at virtual address ffffffc41a0110c8
[    2.783958] Current kworker/u36:0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000022a7000
[    2.792302] [ffffffc41a0110c8] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[    2.800980] Oops [#1]
[    2.803217] Modules linked in:
[    2.806261] CPU: 3 UID: 0 PID: 58 Comm: kworker/u36:0 Not tainted 6.18.0-rc1+ #4 PREEMPTLAZY 
[    2.814763] Hardware name: Banana Pi BPI-F3 (DT)
[    2.819366] Workqueue: events_unbound deferred_probe_work_func
[    2.825180] epc : virt_to_folio+0x5e/0xb8
[    2.829172]  ra : kfree+0x3a/0x528
[    2.832558] epc : ffffffff8034e12e ra : ffffffff8035557a sp : ffffffc600243980
[    2.839762]  gp : ffffffff82074258 tp : ffffffd700994d80 t0 : ffffffff80021540
[    2.846967]  t1 : 0000000000000018 t2 : 2d74696d65636170 s0 : ffffffc600243990
[    2.854172]  s1 : ffffffc600243ab8 a0 : 03ffffc41a0110c0 a1 : ffffffff82123bd0
[    2.861377]  a2 : 7c137c69131cec36 a3 : ffffffff816606d8 a4 : 0000000000000000
[    2.868583]  a5 : ffffffc500000000 a6 : 0000000000000004 a7 : 0000000000000004
[    2.875787]  s2 : ffffffd700b98410 s3 : ffffffc600243ab8 s4 : 0000000000000000
[    2.882991]  s5 : ffffffff80828f1c s6 : 0000000000008437 s7 : ffffffd700b98410
[    2.890197]  s8 : ffffffd700b98410 s9 : ffffffd700900240 s10: ffffffff81fc4100
[    2.897401]  s11: ffffffd700987400 t3 : 0000000000000004 t4 : 0000000000000001
[    2.904607]  t5 : 000000000000001f t6 : 0000000000000003
[    2.909902] status: 0000000200000120 badaddr: ffffffc41a0110c8 cause: 000000000000000d
[    2.917802] [<ffffffff8034e12e>] virt_to_folio+0x5e/0xb8
[    2.923097] [<ffffffff8035557a>] kfree+0x3a/0x528
[    2.927784] [<ffffffff80828f1c>] clk_bulk_put_all+0x64/0x78
[    2.933340] [<ffffffff807249d6>] k1_pcie_phy_probe+0x4ee/0x618
[    2.939155] [<ffffffff808e35e6>] platform_probe+0x56/0x98
[    2.944538] [<ffffffff808e0328>] really_probe+0xa0/0x348
[    2.949832] [<ffffffff808e064c>] __driver_probe_device+0x7c/0x140
[    2.955909] [<ffffffff808e07f8>] driver_probe_device+0x38/0xd0
[    2.961724] [<ffffffff808e0912>] __device_attach_driver+0x82/0xf0
[    2.967801] [<ffffffff808dde6a>] bus_for_each_drv+0x72/0xd0
[    2.973356] [<ffffffff808e0cac>] __device_attach+0x94/0x198
[    2.978912] [<ffffffff808e0fca>] device_initial_probe+0x1a/0x30
[    2.984815] [<ffffffff808defee>] bus_probe_device+0x96/0xa0
[    2.990370] [<ffffffff808dff0e>] deferred_probe_work_func+0xa6/0x110
[    2.996707] [<ffffffff8005cb66>] process_one_work+0x15e/0x340
[    3.002436] [<ffffffff8005d58c>] worker_thread+0x22c/0x348
[    3.007905] [<ffffffff80066b7c>] kthread+0x10c/0x208
[    3.012853] [<ffffffff80014de0>] ret_from_fork_kernel+0x18/0x1c0
[    3.018843] [<ffffffff80c917d6>] ret_from_fork_kernel_asm+0x16/0x18
[    3.025098] Code: 7a98 8d19 2717 0131 3703 5fa7 8131 8d19 051a 953e (651c) f713 
[    3.032497] ---[ end trace 0000000000000000 ]---

It seems that we want clk_bulk_put() and not clk_bulk_put_all(). The 
latter free the clocks, while they have been allocated on the stack.

Regards,
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Aurelien Jarno]

Hi Alex,

On 2025-10-13 10:35, Alex Elder wrote:
> This series introduces a PHY driver and a PCIe driver to support PCIe
> on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
> Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
> PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
> one PCIe lane, and the other two ports each have two lanes.  All PCIe
> ports operate at 5 GT/second.
> 
> The PCIe PHYs must be configured using a value that can only be
> determined using the combo PHY, operating in PCIe mode.  To allow
> that PHY to be used for USB, the calibration step is performed by
> the PHY driver automatically at probe time.  Once this step is done,
> the PHY can be used for either PCIe or USB.
> 
> Version 2 of this series incorporates suggestions made during the
> review of version 1.  Specific highlights are detailed below.

With the issues mentioned in patch 4 fixed, this patchset works fine for 
me. That said I had to disable ASPM by passing pcie_aspm=off on the 
command line, as it is now enabled by default since 6.18-rc1 [1]. At 
this stage, I am not sure if it is an issue with my NVME drive or an 
issue with the controller.

Regards
Aurelien

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3ac2ff14834a0aa056ee3ae0e4b8c641c579961

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v2 1/7] dt-bindings: phy: spacemit: add SpacemiT PCIe/combo PHY

[Alex Elder]

On 10/15/25 9:52 AM, Rob Herring wrote:
>> +  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
>> +      - description: PHY reset; must be deasserted for PHY to function
>> +
>> +  reset-names:
>> +    items:
>> +      - const: dbi
>> +      - const: mstr
>> +      - const: slv
>> +      - const: phy
> I think phy should be first as that's the main one to the phy and the
> others are somewhat questionable. Otherwise,
> 
> Reviewed-by: Rob Herring (Arm)<robh@kernel.org>

OK I'll arrange them that way in v3.  Thank you.	-Alex

Re: [PATCH v2 2/7] dt-bindings: phy: spacemit: introduce PCIe PHY

[Alex Elder]

On 10/15/25 11:41 AM, Rob Herring (Arm) wrote:
> 
> On Mon, 13 Oct 2025 10:35:19 -0500, Alex Elder wrote:
>> Add the Device Tree binding for two PCIe PHYs present on the SpacemiT
>> K1 SoC.  These PHYs are dependent on a separate combo PHY, which
>> determines at probe time the calibration values used by the PCIe-only
>> PHYs.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - Added '>' to the description, and reworded it a bit
>>      - Added clocks and clock-names properties
>>      - Dropped the label and status property from the example
>>
>>   .../bindings/phy/spacemit,k1-pcie-phy.yaml    | 59 +++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/spacemit,k1-pcie-phy.yaml
>>
> 
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Thanks.	-Alex

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Alex Elder]

On 10/15/25 11:47 AM, Rob Herring wrote:
> On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
>> Add the Device Tree binding for the PCIe root complex found on the
>> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
>> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
>> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
>> typically used to support a USB 3 port.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - Renamed the binding, using "host controller"
>>      - Added '>' to the description, and reworded it a bit
>>      - Added reference to /schemas/pci/snps,dw-pcie.yaml
>>      - Fixed and renamed the compatible string
>>      - Renamed the PMU property, and fixed its description
>>      - Consistently omit the period at the end of descriptions
>>      - Renamed the "global" clock to be "phy"
>>      - Use interrupts rather than interrupts-extended, and name the
>>        one interrupt "msi" to make clear its purpose
>>      - Added a vpcie3v3-supply property
>>      - Dropped the max-link-speed property
>>      - Changed additionalProperties to unevaluatedProperties
>>      - Dropped the label and status property from the example
>>
>>   .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>>   1 file changed, 156 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> new file mode 100644
>> index 0000000000000..87745d49c53a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> @@ -0,0 +1,156 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: SpacemiT K1 PCI Express Host Controller
>> +
>> +maintainers:
>> +  - Alex Elder <elder@riscstar.com>
>> +
>> +description: >
>> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
>> +  DesignWare 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
>> +
>> +  reg:
>> +    items:
>> +      - description: DesignWare PCIe registers
>> +      - description: ATU address space
>> +      - description: PCIe configuration space
>> +      - description: Link control registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: dbi
>> +      - const: atu
>> +      - const: config
>> +      - const: link
>> +
>> +  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
>> +
>> +  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
>> +      - description: Global reset; must be deasserted for PHY to function
>> +
>> +  reset-names:
>> +    items:
>> +      - const: dbi
>> +      - const: mstr
>> +      - const: slv
>> +      - const: phy
> 
> You expect/need the phy driver and PCIe driver to both reset the PHY?
> You should do that indirectly with the PHY API when you reset the
> controller.
This was previously called the "global" reset, and I renamed it
to align with one of the existing DWC "app" resets.

I put it here because I had the impression that it was required
to be deasserted for both PCIe and the PHY to function.  Currently
only the combo PHY gets and deasserts the PHY reset (for the
benefit of USB).

Instead, I'll require this global/phy reset for both the combo
PHY and the PCIe PHYs.  I will get it (deasserted) during probe
for all of them.  Then I'll remove it from the list of resets
required/managed for PCIe ports.

I'm going to keep your Reviewed-by on patch 2, even though I'll
be adding "reset" and "reset-names" as required properties.

Please tell me if you'd like me not to do that.

Thanks for the review.

					-Alex

> Rob

Re: [PATCH v2 4/7] phy: spacemit: introduce PCIe/combo PHY

[Alex Elder]

On 10/15/25 4:51 PM, Aurelien Jarno wrote:
> Hi,
> 
> On 2025-10-13 10:35, Alex Elder wrote:
>> Introduce a driver that supports three PHYs found on the SpacemiT
>> K1 SoC.  The first PHY is a combo PHY that can be configured for
>> use for either USB 3 or PCIe.  The other two PHYs support PCIe
>> only.
>>
>> All three PHYs must be programmed with an 8 bit receiver termination
>> value, which must be determined dynamically.  Only the combo PHY is
>> able to determine this value.  The combo PHY performs a special
>> calibration step at probe time to discover this, and that value is
>> used to program each PHY that operates in PCIe mode.  The combo
>> PHY must therefore be probed before either of the PCIe-only PHYs
>> will be used.
>>
>> Each PHY has an internal PLL driven from an external oscillator.
>> This PLL started when the PHY is first initialized, and stays
>> on thereafter.
>>
>> During normal operation, the USB or PCIe driver using the PHY must
>> ensure (other) clocks and resets are set up properly.
>>
>> However PCIe mode clocks are enabled and resets are de-asserted
>> temporarily by this driver to perform the calibration step on the
>> combo PHY.
>>
>> Tested-by: Junzhong Pan <panjunzhong@linux.spacemit.com>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
> 
> Thanks for this new version. I have tried it on top of v6.18-rc1 +
> spacemit DTS commits from next on a BPI-F3, and it fails calibrating the
> PHY with:

I don't see this on my BPI-F3, but I now understand why.

> [    2.748405] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: calibration failed
> [    2.755300] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: error probing combo phy
> [    2.763088] spacemit-k1-pcie-phy c0b10000.phy: probe with driver spacemit-k1-pcie-phy failed with error -2
> [   14.309031] platform c0d10000.phy: deferred probe pending: (reason unknown)
> [   14.313426] platform c0c10000.phy: deferred probe pending: (reason unknown)
> [   14.320347] platform ca400000.pcie: deferred probe pending: platform: supplier c0c10000.phy not ready
> [   14.329542] platform ca800000.pcie: deferred probe pending: platform: supplier c0d10000.phy not ready
> 
> Note that version 1 was working fine on the same board.
> 
> [ snip ]
> 
>> diff --git a/drivers/phy/phy-spacemit-k1-pcie.c b/drivers/phy/phy-spacemit-k1-pcie.c
>> new file mode 100644
>> index 0000000000000..81bc05823d080
>> --- /dev/null
>> +++ b/drivers/phy/phy-spacemit-k1-pcie.c
> 
> [ snip ]
> 
>> +static int k1_pcie_combo_phy_calibrate(struct k1_pcie_phy *k1_phy)
>> +{
>> +	struct reset_control_bulk_data resets[] = {
>> +		{ .id = "dbi", },
>> +		{ .id = "mstr", },
>> +		{ .id = "slv", },
>> +	};
>> +	struct clk_bulk_data clocks[] = {
>> +		{ .id = "dbi", },
>> +		{ .id = "mstr", },
>> +		{ .id = "slv", },
>> +	};
>> +	struct device *dev = k1_phy->dev;
>> +	struct reset_control *phy_reset;
>> +	int ret = 0;
>> +	int val;
>> +
>> +	/* Nothing to do if we already set the receiver termination value */
>> +	if (k1_phy_rterm_valid())
>> +		return 0;
>> +
>> +	/* De-assert the PHY (global) reset and leave it that way for USB */
>> +	phy_reset = devm_reset_control_get_exclusive_deasserted(dev, "phy");
>> +	if (IS_ERR(phy_reset))
>> +		return PTR_ERR(phy_reset);
>> +
>> +	/*
>> +	 * We also guarantee the APP_HOLD_PHY_RESET bit is clear.  We can
>> +	 * leave this bit clear even if an error happens below.
>> +	 */
>> +	regmap_assign_bits(k1_phy->pmu, PCIE_CLK_RES_CTRL,
>> +			   PCIE_APP_HOLD_PHY_RST, false);
>> +
>> +	/* If the calibration already completed (e.g. by U-Boot), we're done */
>> +	val = readl(k1_phy->regs + PCIE_RCAL_RESULT);
>> +	if (val & R_TUNE_DONE)
>> +		goto out_tune_done;
I refer to the above three lines, below.

>> +	/* Put the PHY into PCIe mode */
>> +	k1_combo_phy_sel(k1_phy, false);
>> +
>> +	/* Get and enable the PCIe app clocks */
>> +	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
>> +	if (ret <= 0) {
>> +		if (!ret)
>> +			ret = -ENOENT;
>> +		goto out_tune_done;
>> +	}
> 
> This part doesn't look correct. The documentation says this function
> "returns 0 if all clocks specified in clk_bulk_data table are obtained
> successfully, or valid IS_ERR() condition containing errno."
> 
> To me, it seems the code should only be:
> 
> 	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
> 	if (ret)
> 		goto out_tune_done;

OK I understand the problem here.

On v1 of the series, this used clk_bulk_get_all(), and I changed
that to clk_bulk_get().  There is now an additional reference
clock defined, used by the PLL clock.  So here we only want to
get three clocks, not that new one.

The return value from clk_bulk_get_all() is the number of clocks,
and 0 means "not found".  So I guess I neglected to change the
handling of the return value here when I changed the function.

The reason I didn't see this is that calibration only ever has
to happen once (per boot).  In the lines I noted earlier, the
PCIE_RCAL_RESULT register is read, and if the R_TUNE_DONE bit
is already set, calibration is complete (probably done by the
boot loader).

In my case, calibration was already done, so I never had to get
the clock or resets, etc.  In your case, this driver had to do
the calibration, so you (successfully) got the clocks, and
that (erroneously) resulted in an error.

I reproduced the problem(s) you observed by forcing the
calibration on my machine.

Thank you very much for reporting this.  Your fix is correct,
and I will include it in v3 of the series.

> [snip]
> 
>> +out_put_clocks:
>> +	clk_bulk_put_all(ARRAY_SIZE(clocks), clocks);
> 
> When fixing the above bug, this then crashes with:
> 
> [    2.776109] Unable to handle kernel paging request at virtual address ffffffc41a0110c8
> [    2.783958] Current kworker/u36:0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000022a7000
> [    2.792302] [ffffffc41a0110c8] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
> [    2.800980] Oops [#1]
> [    2.803217] Modules linked in:
> [    2.806261] CPU: 3 UID: 0 PID: 58 Comm: kworker/u36:0 Not tainted 6.18.0-rc1+ #4 PREEMPTLAZY
> [    2.814763] Hardware name: Banana Pi BPI-F3 (DT)
> [    2.819366] Workqueue: events_unbound deferred_probe_work_func
> [    2.825180] epc : virt_to_folio+0x5e/0xb8
> [    2.829172]  ra : kfree+0x3a/0x528
> [    2.832558] epc : ffffffff8034e12e ra : ffffffff8035557a sp : ffffffc600243980
> [    2.839762]  gp : ffffffff82074258 tp : ffffffd700994d80 t0 : ffffffff80021540
> [    2.846967]  t1 : 0000000000000018 t2 : 2d74696d65636170 s0 : ffffffc600243990
> [    2.854172]  s1 : ffffffc600243ab8 a0 : 03ffffc41a0110c0 a1 : ffffffff82123bd0
> [    2.861377]  a2 : 7c137c69131cec36 a3 : ffffffff816606d8 a4 : 0000000000000000
> [    2.868583]  a5 : ffffffc500000000 a6 : 0000000000000004 a7 : 0000000000000004
> [    2.875787]  s2 : ffffffd700b98410 s3 : ffffffc600243ab8 s4 : 0000000000000000
> [    2.882991]  s5 : ffffffff80828f1c s6 : 0000000000008437 s7 : ffffffd700b98410
> [    2.890197]  s8 : ffffffd700b98410 s9 : ffffffd700900240 s10: ffffffff81fc4100
> [    2.897401]  s11: ffffffd700987400 t3 : 0000000000000004 t4 : 0000000000000001
> [    2.904607]  t5 : 000000000000001f t6 : 0000000000000003
> [    2.909902] status: 0000000200000120 badaddr: ffffffc41a0110c8 cause: 000000000000000d
> [    2.917802] [<ffffffff8034e12e>] virt_to_folio+0x5e/0xb8
> [    2.923097] [<ffffffff8035557a>] kfree+0x3a/0x528
> [    2.927784] [<ffffffff80828f1c>] clk_bulk_put_all+0x64/0x78
> [    2.933340] [<ffffffff807249d6>] k1_pcie_phy_probe+0x4ee/0x618
> [    2.939155] [<ffffffff808e35e6>] platform_probe+0x56/0x98
> [    2.944538] [<ffffffff808e0328>] really_probe+0xa0/0x348
> [    2.949832] [<ffffffff808e064c>] __driver_probe_device+0x7c/0x140
> [    2.955909] [<ffffffff808e07f8>] driver_probe_device+0x38/0xd0
> [    2.961724] [<ffffffff808e0912>] __device_attach_driver+0x82/0xf0
> [    2.967801] [<ffffffff808dde6a>] bus_for_each_drv+0x72/0xd0
> [    2.973356] [<ffffffff808e0cac>] __device_attach+0x94/0x198
> [    2.978912] [<ffffffff808e0fca>] device_initial_probe+0x1a/0x30
> [    2.984815] [<ffffffff808defee>] bus_probe_device+0x96/0xa0
> [    2.990370] [<ffffffff808dff0e>] deferred_probe_work_func+0xa6/0x110
> [    2.996707] [<ffffffff8005cb66>] process_one_work+0x15e/0x340
> [    3.002436] [<ffffffff8005d58c>] worker_thread+0x22c/0x348
> [    3.007905] [<ffffffff80066b7c>] kthread+0x10c/0x208
> [    3.012853] [<ffffffff80014de0>] ret_from_fork_kernel+0x18/0x1c0
> [    3.018843] [<ffffffff80c917d6>] ret_from_fork_kernel_asm+0x16/0x18
> [    3.025098] Code: 7a98 8d19 2717 0131 3703 5fa7 8131 8d19 051a 953e (651c) f713
> [    3.032497] ---[ end trace 0000000000000000 ]---
> 
> It seems that we want clk_bulk_put() and not clk_bulk_put_all(). The
> latter free the clocks, while they have been allocated on the stack.

Yes, you are correct.  This too is an artifact of me doing a bad job
switching to the clk_bulk_get() interface.

Thanks again for your message.  Reviews are awesome, but someone
actually testing it is a good way to find problems like this.

					-Alex

> 
> Regards,
> Aurelien
> 

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Alex Elder]

On 10/16/25 11:47 AM, Aurelien Jarno wrote:
> Hi Alex,
> 
> On 2025-10-13 10:35, Alex Elder wrote:
>> This series introduces a PHY driver and a PCIe driver to support PCIe
>> on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
>> Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
>> PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
>> one PCIe lane, and the other two ports each have two lanes.  All PCIe
>> ports operate at 5 GT/second.
>>
>> The PCIe PHYs must be configured using a value that can only be
>> determined using the combo PHY, operating in PCIe mode.  To allow
>> that PHY to be used for USB, the calibration step is performed by
>> the PHY driver automatically at probe time.  Once this step is done,
>> the PHY can be used for either PCIe or USB.
>>
>> Version 2 of this series incorporates suggestions made during the
>> review of version 1.  Specific highlights are detailed below.
> 
> With the issues mentioned in patch 4 fixed, this patchset works fine for
> me. That said I had to disable ASPM by passing pcie_aspm=off on the
> command line, as it is now enabled by default since 6.18-rc1 [1]. At
> this stage, I am not sure if it is an issue with my NVME drive or an
> issue with the controller.

Can you describe what symptoms you had that required you to pass
"pcie_aspm=off" on the kernel command line?

I see these lines in my boot log related to ASPM (and added by
the commit you link to), for both pcie1 and pcie2:

   pci 0000:01:00.0: ASPM: DT platform, enabling L0s-up L0s-dw L1 AS
PM-L1.1 ASPM-L1.2 PCI-PM-L1.1 PCI-PM-L1.2
   pci 0000:01:00.0: ASPM: DT platform, enabling ClockPM

   . . .

   nvme nvme0: pci function 0000:01:00.0
   nvme 0000:01:00.0: enabling device (0000 -> 0002)
   nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
   nvme nvme0: 8/0/0 default/read/poll queues
    nvme0n1: p1

My NVMe drive on pcie1 works correctly.
   https://www.crucial.com/ssd/p3/CT1000P3SSD8

   root@bananapif3:~# df /a
   Filesystem     1K-blocks     Used Available Use% Mounted on
   /dev/nvme0n1p1 960302804 32063304 879385040   4% /a
   root@bananapif3:~#

I basically want to know if there's something I should do with this
driver to address this.  (Mani, can you explain?)

Thank you.

					-Alex

> Regards
> Aurelien
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3ac2ff14834a0aa056ee3ae0e4b8c641c579961
> 

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Manivannan Sadhasivam]

On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
> Add the Device Tree binding for the PCIe root complex found on the
> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
> typically used to support a USB 3 port.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Renamed the binding, using "host controller"
>     - Added '>' to the description, and reworded it a bit
>     - Added reference to /schemas/pci/snps,dw-pcie.yaml
>     - Fixed and renamed the compatible string
>     - Renamed the PMU property, and fixed its description
>     - Consistently omit the period at the end of descriptions
>     - Renamed the "global" clock to be "phy"
>     - Use interrupts rather than interrupts-extended, and name the
>       one interrupt "msi" to make clear its purpose
>     - Added a vpcie3v3-supply property
>     - Dropped the max-link-speed property
>     - Changed additionalProperties to unevaluatedProperties
>     - Dropped the label and status property from the example
> 
>  .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> new file mode 100644
> index 0000000000000..87745d49c53a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 PCI Express Host Controller
> +
> +maintainers:
> +  - Alex Elder <elder@riscstar.com>
> +
> +description: >
> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
> +  DesignWare 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
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: ATU address space
> +      - description: PCIe configuration space
> +      - description: Link control registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: atu
> +      - const: config
> +      - const: link
> +
> +  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
> +
> +  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
> +      - description: Global reset; must be deasserted for PHY to function
> +
> +  reset-names:
> +    items:
> +      - const: dbi
> +      - const: mstr
> +      - const: slv
> +      - const: phy
> +
> +  interrupts:
> +    items:
> +      - description: Interrupt used for MSIs
> +
> +  interrupt-names:
> +    const: msi
> +
> +  phys:
> +    maxItems: 1
> +
> +  vpcie3v3-supply:
> +    description:
> +      A phandle for 3.3v regulator to use for PCIe

Could you please move these Root Port specific properties (phy, vpcie3v3-supply)
to the Root Port node?

Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml

For handling the 'vpcie3v3-supply', you can rely on PCI_PWRCTRL_SLOT driver.

> +
> +  device_type:
> +    const: pci
> +

This is part of the PCI bus schema itself.

> +  num-viewport:
> +    const: 8
> +

This property has been deprecated in favor of driver auto-detecting the iATU
regions.

- Mani

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

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Manivannan Sadhasivam]

On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
> Introduce a driver for the PCIe host controller found in the SpacemiT
> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> The driver supports three PCIe ports that operate at PCIe gen2 transfer
> rates (5 GT/sec).  The first port uses a combo PHY, which may be
> configured for use for USB 3 instead.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: - Renamed the PCIe driver source file "pcie-spacemit-k1.c"
>     - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1; it
>       is now tristate rather than Boolean
>     - The PCIe host compatible string is now "spacemit,k1-pcie"
>     - Renamed the PMU syscon property to be "spacemit,apmu"
>     - Renamed the symbols representing the PCI vendor and device IDs
>       to align with <linux/pci_ids.h>
>     - Use PCIE_T_PVPERL_MS rather than 100 to represent a standard
>       delay period.
>     - Use platform (not dev) driver-data access functions; assignment
>       is done only after the private structure is initialized
>     - Deleted some unneeded includes in the PCIe driver.
>     - Dropped error checking when operating on MMIO-backed regmaps
>     - Added a regmap_read() call in two places, to ensure a specified
>       delay occurs *after* the a MMIO write has reached its target.
>     - Used ARRAY_SIZE() (not a local variable value) in a few spots
>     - Now use readl_relaxed()/writel_relaxed() when operating on
>       the "link" I/O memory space in the PCIe driver
>     - Updated a few error messages for consistency
>     - No longer specify suppress_bind_attrs in the PCIe driver
>     - Now specify PCIe driver probe type as PROBE_PREFER_ASYNCHRONOUS
>     - No longer use (void) cast to indicate ignored return values
> 
>  drivers/pci/controller/dwc/Kconfig            |  10 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-spacemit-k1.c | 319 ++++++++++++++++++
>  3 files changed, 330 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-spacemit-k1.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 349d4657393c9..ede59b34c99ba 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -509,6 +509,16 @@ config PCI_KEYSTONE_EP
>  	  on DesignWare hardware and therefore the driver re-uses the
>  	  DesignWare core functions to implement the driver.
>  
> +config PCIE_SPACEMIT_K1
> +	tristate "SpacemiT K1 PCIe controller (host mode)"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on PCI && OF && HAS_IOMEM
> +	select PCIE_DW_HOST
> +	default ARCH_SPACEMIT
> +	help
> +	  Enables support for the PCIe controller in the K1 SoC operating
> +	  in host mode.
> +
>  config PCIE_VISCONTI_HOST
>  	bool "Toshiba Visconti PCIe controller"
>  	depends on ARCH_VISCONTI || COMPILE_TEST
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 7ae28f3b0fb39..662b0a219ddc4 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> +obj-$(CONFIG_PCIE_SPACEMIT_K1) += pcie-spacemit-k1.o
>  obj-$(CONFIG_PCIE_STM32_HOST) += pcie-stm32.o
>  obj-$(CONFIG_PCIE_STM32_EP) += pcie-stm32-ep.o
>  
> diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> new file mode 100644
> index 0000000000000..d58232cbb8a02
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SpacemiT K1 PCIe host driver
> + *
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + * Copyright (c) 2023, spacemit Corporation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/types.h>
> +
> +#include "pcie-designware.h"
> +
> +#define PCI_VENDOR_ID_SPACEMIT		0x201f
> +#define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
> +
> +/* Offsets and field definitions for link management registers */
> +

nit: drop the extra newline

> +#define K1_PHY_AHB_IRQ_EN			0x0000
> +#define PCIE_INTERRUPT_EN		BIT(0)
> +
> +#define K1_PHY_AHB_LINK_STS			0x0004
> +#define SMLH_LINK_UP			BIT(1)
> +#define RDLH_LINK_UP			BIT(12)
> +
> +#define INTR_ENABLE				0x0014
> +#define MSI_CTRL_INT			BIT(11)
> +
> +/* Some controls require APMU regmap access */
> +#define SYSCON_APMU			"spacemit,apmu"
> +
> +/* Offsets and field definitions for APMU registers */
> +

here also

> +#define PCIE_CLK_RESET_CONTROL			0x0000
> +#define LTSSM_EN			BIT(6)
> +#define PCIE_AUX_PWR_DET		BIT(9)
> +#define PCIE_RC_PERST			BIT(12)	/* 1: assert PERST# */
> +#define APP_HOLD_PHY_RST		BIT(30)
> +#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
> +
> +#define PCIE_CONTROL_LOGIC			0x0004
> +#define PCIE_SOFT_RESET			BIT(0)
> +
> +struct k1_pcie {
> +	struct dw_pcie pci;
> +	struct phy *phy;
> +	void __iomem *link;
> +	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
> +	u32 pmu_off;
> +};
> +
> +#define to_k1_pcie(dw_pcie) \
> +		platform_get_drvdata(to_platform_device((dw_pcie)->dev))
> +
> +static void k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
> +{
> +	u32 offset;
> +	u32 val;
> +
> +	/*
> +	 * Write, then read back to guarantee it has reached the device
> +	 * before we start the delay.
> +	 */
> +	offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
> +	regmap_set_bits(k1->pmu, offset, PCIE_SOFT_RESET);
> +	regmap_read(k1->pmu, offset, &val);
> +
> +	mdelay(2);
> +
> +	regmap_clear_bits(k1->pmu, offset, PCIE_SOFT_RESET);
> +}
> +
> +/* Enable app clocks, deassert resets */
> +static int k1_pcie_activate(struct k1_pcie *k1)

k1_pcie_enable_resources()?

> +{
> +	struct dw_pcie *pci = &k1->pci;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->app_rsts),
> +					  pci->app_rsts);
> +	if (ret)
> +		goto err_disable_clks;
> +
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->core_rsts),
> +					  pci->core_rsts);
> +	if (ret)
> +		goto err_assert_resets;
> +
> +	return 0;
> +
> +err_assert_resets:
> +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
> +err_disable_clks:
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> +
> +	return ret;
> +}
> +
> +/* Assert resets, disable app clocks */
> +static void k1_pcie_deactivate(struct k1_pcie *k1)

k1_pcie_disable_resources()?

> +{
> +	struct dw_pcie *pci = &k1->pci;
> +
> +	reset_control_bulk_assert(ARRAY_SIZE(pci->core_rsts), pci->core_rsts);
> +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> +}
> +
> +static int k1_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 offset;
> +	u32 mask;
> +	u32 val;
> +	int ret;
> +
> +	k1_pcie_toggle_soft_reset(k1);
> +
> +	ret = k1_pcie_activate(k1);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_init(k1->phy);
> +	if (ret) {
> +		k1_pcie_deactivate(k1);
> +
> +		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);
> +
> +	/*
> +	 * Assert fundamental reset (drive PERST# low).  Put the port in

s/port/controller

> +	 * root complex mode, and indicate that Vaux (3.3v) is present.
> +	 */
> +	mask = PCIE_RC_PERST;
> +	mask |= DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
> +
> +	/*
> +	 * Write, then read back to guarantee it has reached the device
> +	 * before we start the delay.
> +	 */
> +	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
> +	regmap_set_bits(k1->pmu, offset, mask);
> +	regmap_read(k1->pmu, offset, &val);
> +
> +	mdelay(PCIE_T_PVPERL_MS);
> +
> +	/* Deassert fundamental reset (drive PERST# high) */
> +	regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
> +
> +	return 0;
> +}
> +
> +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);
> +
> +	/* Assert fundamental reset (drive PERST# low) */
> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			PCIE_RC_PERST);

You need assert PERST# here.

> +
> +	phy_exit(k1->phy);
> +
> +	k1_pcie_deactivate(k1);
> +}
> +
> +static const struct dw_pcie_host_ops k1_pcie_host_ops = {
> +	.init		= k1_pcie_init,
> +	.deinit		= k1_pcie_deinit,
> +};
> +
> +static bool k1_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 val;
> +
> +	val = readl_relaxed(k1->link + K1_PHY_AHB_LINK_STS);
> +
> +	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
> +}
> +
> +static int k1_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 val;
> +
> +	/* Stop holding the PHY in reset, and enable link training */
> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			   APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
> +
> +	/* Enable the MSI interrupt */
> +	writel_relaxed(MSI_CTRL_INT, k1->link + INTR_ENABLE);
> +
> +	/* Top-level interrupt enable */
> +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
> +	val |= PCIE_INTERRUPT_EN;
> +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
> +
> +	return 0;
> +}
> +
> +static void k1_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 val;
> +
> +	/* Disable interrupts */
> +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
> +	val &= ~PCIE_INTERRUPT_EN;
> +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
> +
> +	writel_relaxed(0, k1->link + INTR_ENABLE);
> +
> +	/* Disable the link and hold the PHY in reset */
> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			   APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
> +}
> +
> +static const struct dw_pcie_ops k1_pcie_ops = {
> +	.link_up	= k1_pcie_link_up,
> +	.start_link	= k1_pcie_start_link,
> +	.stop_link	= k1_pcie_stop_link,
> +};
> +
> +static int k1_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct k1_pcie *k1;
> +	int ret;
> +
> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
> +	if (!k1)
> +		return -ENOMEM;
> +
> +	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
> +						       SYSCON_APMU, 1,
> +						       &k1->pmu_off);
> +	if (IS_ERR(k1->pmu))
> +		return dev_err_probe(dev, PTR_ERR(k1->pmu),
> +				     "failed to lookup PMU registers\n");
> +
> +	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
> +	if (!k1->link)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "failed to map \"link\" registers\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get \"vpcie3v3\" supply\n");

As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
to handle the power supplies. It is not yet handling the PERST#, but I have a
series floating for that:
https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/

> +
> +	/* Hold the PHY in reset until we start the link */
> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			APP_HOLD_PHY_RST);
> +
> +	k1->phy = devm_phy_get(dev, NULL);
> +	if (IS_ERR(k1->phy))
> +		return dev_err_probe(dev, PTR_ERR(k1->phy),
> +				     "failed to get PHY\n");

Once you move these properties to Root Port binding, you need to have per-Root
Port parser. Again, you can refer the STM32 driver.

> +
> +	k1->pci.dev = dev;
> +	k1->pci.ops = &k1_pcie_ops;
> +	dw_pcie_cap_set(&k1->pci, REQ_RES);
> +
> +	k1->pci.pp.ops = &k1_pcie_host_ops;
> +	k1->pci.pp.num_vectors = MAX_MSI_IRQS;

This driver is just using a single 'msi' vector, which can only support 32 MSIs.
But MAX_MSI_IRQS is 256. So this looks wrong.

> +
> +	platform_set_drvdata(pdev, k1);
> +

For setting the correct runtime PM state of the controller, you should do:

pm_runtime_set_active()
pm_runtime_no_callbacks()
devm_pm_runtime_enable()

This will fix the runtime PM hierarchy of PCIe chain (from host controller to
client drivers). Otherwise, it will be broken.

- Mani

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

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Alex Elder]

On 10/26/25 11:38 AM, Manivannan Sadhasivam wrote:
> On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
>> Add the Device Tree binding for the PCIe root complex found on the
>> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
>> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
>> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
>> typically used to support a USB 3 port.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - Renamed the binding, using "host controller"
>>      - Added '>' to the description, and reworded it a bit
>>      - Added reference to /schemas/pci/snps,dw-pcie.yaml
>>      - Fixed and renamed the compatible string
>>      - Renamed the PMU property, and fixed its description
>>      - Consistently omit the period at the end of descriptions
>>      - Renamed the "global" clock to be "phy"
>>      - Use interrupts rather than interrupts-extended, and name the
>>        one interrupt "msi" to make clear its purpose
>>      - Added a vpcie3v3-supply property
>>      - Dropped the max-link-speed property
>>      - Changed additionalProperties to unevaluatedProperties
>>      - Dropped the label and status property from the example
>>
>>   .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>>   1 file changed, 156 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> new file mode 100644
>> index 0000000000000..87745d49c53a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>> @@ -0,0 +1,156 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: SpacemiT K1 PCI Express Host Controller
>> +
>> +maintainers:
>> +  - Alex Elder <elder@riscstar.com>
>> +
>> +description: >
>> +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
>> +  DesignWare 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
>> +
>> +  reg:
>> +    items:
>> +      - description: DesignWare PCIe registers
>> +      - description: ATU address space
>> +      - description: PCIe configuration space
>> +      - description: Link control registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: dbi
>> +      - const: atu
>> +      - const: config
>> +      - const: link
>> +
>> +  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
>> +
>> +  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
>> +      - description: Global reset; must be deasserted for PHY to function
>> +
>> +  reset-names:
>> +    items:
>> +      - const: dbi
>> +      - const: mstr
>> +      - const: slv
>> +      - const: phy
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Interrupt used for MSIs
>> +
>> +  interrupt-names:
>> +    const: msi
>> +
>> +  phys:
>> +    maxItems: 1
>> +
>> +  vpcie3v3-supply:
>> +    description:
>> +      A phandle for 3.3v regulator to use for PCIe
> 
> Could you please move these Root Port specific properties (phy, vpcie3v3-supply)
> to the Root Port node?
> 
> Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml

OK, I'll try to follow what that ST binding does (and the
matching driver).

> For handling the 'vpcie3v3-supply', you can rely on PCI_PWRCTRL_SLOT driver.
I looked at the code under pci/pwrctrl.  But is there some other
documentation I should be looking at for this?

It looks like it involves creating a new node compatible with
"pciclass,0604".  And that the purpose of that driver was to
ensure certain resources are enabled before the "real" PCI
device gets probed.

I see two arm64 DTS files using it:  x1e80100.dtsi and r8a779g0.dtsi.
Both define this node inside the main PCIe controller node.

Will this model (with the parent pwrctrl node and child PCI
controller node) be used for all PCI controllers from here on?

Or are you saying this properly represents the relationship of
the supply with the PCIe port in this SpacemiT case?

>> +
>> +  device_type:
>> +    const: pci
>> +
> 
> This is part of the PCI bus schema itself.

That means I don't have to specify it here.  I'll remove it.
I will also remove it from the list of required properties.

>> +  num-viewport:
>> +    const: 8
>> +
> 
> This property has been deprecated in favor of driver auto-detecting the iATU
> regions.

Yes, that got removed in v3 of the series.

Thanks.

					-Alex

> 
> - Mani
> 

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Alex Elder]

On 10/26/25 11:55 AM, Manivannan Sadhasivam wrote:
> On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
>> Introduce a driver for the PCIe host controller found in the SpacemiT
>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>> The driver supports three PCIe ports that operate at PCIe gen2 transfer
>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>> configured for use for USB 3 instead.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v2: - Renamed the PCIe driver source file "pcie-spacemit-k1.c"
>>      - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1; it
>>        is now tristate rather than Boolean
>>      - The PCIe host compatible string is now "spacemit,k1-pcie"
>>      - Renamed the PMU syscon property to be "spacemit,apmu"
>>      - Renamed the symbols representing the PCI vendor and device IDs
>>        to align with <linux/pci_ids.h>
>>      - Use PCIE_T_PVPERL_MS rather than 100 to represent a standard
>>        delay period.
>>      - Use platform (not dev) driver-data access functions; assignment
>>        is done only after the private structure is initialized
>>      - Deleted some unneeded includes in the PCIe driver.
>>      - Dropped error checking when operating on MMIO-backed regmaps
>>      - Added a regmap_read() call in two places, to ensure a specified
>>        delay occurs *after* the a MMIO write has reached its target.
>>      - Used ARRAY_SIZE() (not a local variable value) in a few spots
>>      - Now use readl_relaxed()/writel_relaxed() when operating on
>>        the "link" I/O memory space in the PCIe driver
>>      - Updated a few error messages for consistency
>>      - No longer specify suppress_bind_attrs in the PCIe driver
>>      - Now specify PCIe driver probe type as PROBE_PREFER_ASYNCHRONOUS
>>      - No longer use (void) cast to indicate ignored return values
>>
>>   drivers/pci/controller/dwc/Kconfig            |  10 +
>>   drivers/pci/controller/dwc/Makefile           |   1 +
>>   drivers/pci/controller/dwc/pcie-spacemit-k1.c | 319 ++++++++++++++++++
>>   3 files changed, 330 insertions(+)
>>   create mode 100644 drivers/pci/controller/dwc/pcie-spacemit-k1.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 349d4657393c9..ede59b34c99ba 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -509,6 +509,16 @@ config PCI_KEYSTONE_EP
>>   	  on DesignWare hardware and therefore the driver re-uses the
>>   	  DesignWare core functions to implement the driver.
>>   
>> +config PCIE_SPACEMIT_K1
>> +	tristate "SpacemiT K1 PCIe controller (host mode)"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on PCI && OF && HAS_IOMEM
>> +	select PCIE_DW_HOST
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enables support for the PCIe controller in the K1 SoC operating
>> +	  in host mode.
>> +
>>   config PCIE_VISCONTI_HOST
>>   	bool "Toshiba Visconti PCIe controller"
>>   	depends on ARCH_VISCONTI || COMPILE_TEST
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 7ae28f3b0fb39..662b0a219ddc4 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>>   obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>>   obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>>   obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
>> +obj-$(CONFIG_PCIE_SPACEMIT_K1) += pcie-spacemit-k1.o
>>   obj-$(CONFIG_PCIE_STM32_HOST) += pcie-stm32.o
>>   obj-$(CONFIG_PCIE_STM32_EP) += pcie-stm32-ep.o
>>   
>> diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
>> new file mode 100644
>> index 0000000000000..d58232cbb8a02
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
>> @@ -0,0 +1,319 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 PCIe host driver
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + * Copyright (c) 2023, spacemit Corporation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/gfp.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/types.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define PCI_VENDOR_ID_SPACEMIT		0x201f
>> +#define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
>> +
>> +/* Offsets and field definitions for link management registers */
>> +
> 
> nit: drop the extra newline

OK.

>> +#define K1_PHY_AHB_IRQ_EN			0x0000
>> +#define PCIE_INTERRUPT_EN		BIT(0)
>> +
>> +#define K1_PHY_AHB_LINK_STS			0x0004
>> +#define SMLH_LINK_UP			BIT(1)
>> +#define RDLH_LINK_UP			BIT(12)
>> +
>> +#define INTR_ENABLE				0x0014
>> +#define MSI_CTRL_INT			BIT(11)
>> +
>> +/* Some controls require APMU regmap access */
>> +#define SYSCON_APMU			"spacemit,apmu"
>> +
>> +/* Offsets and field definitions for APMU registers */
>> +
> 
> here also

OK.

>> +#define PCIE_CLK_RESET_CONTROL			0x0000
>> +#define LTSSM_EN			BIT(6)
>> +#define PCIE_AUX_PWR_DET		BIT(9)
>> +#define PCIE_RC_PERST			BIT(12)	/* 1: assert PERST# */
>> +#define APP_HOLD_PHY_RST		BIT(30)
>> +#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
>> +
>> +#define PCIE_CONTROL_LOGIC			0x0004
>> +#define PCIE_SOFT_RESET			BIT(0)
>> +
>> +struct k1_pcie {
>> +	struct dw_pcie pci;
>> +	struct phy *phy;
>> +	void __iomem *link;
>> +	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
>> +	u32 pmu_off;
>> +};
>> +
>> +#define to_k1_pcie(dw_pcie) \
>> +		platform_get_drvdata(to_platform_device((dw_pcie)->dev))
>> +
>> +static void k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
>> +{
>> +	u32 offset;
>> +	u32 val;
>> +
>> +	/*
>> +	 * Write, then read back to guarantee it has reached the device
>> +	 * before we start the delay.
>> +	 */
>> +	offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
>> +	regmap_set_bits(k1->pmu, offset, PCIE_SOFT_RESET);
>> +	regmap_read(k1->pmu, offset, &val);
>> +
>> +	mdelay(2);
>> +
>> +	regmap_clear_bits(k1->pmu, offset, PCIE_SOFT_RESET);
>> +}
>> +
>> +/* Enable app clocks, deassert resets */
>> +static int k1_pcie_activate(struct k1_pcie *k1)
> 
> k1_pcie_enable_resources()?

OK, I'll use k1_pcie_{enable,disable}_resources().

>> +{
>> +	struct dw_pcie *pci = &k1->pci;
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(pci->app_clks), pci->app_clks);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->app_rsts),
>> +					  pci->app_rsts);
>> +	if (ret)
>> +		goto err_disable_clks;
>> +
>> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->core_rsts),
>> +					  pci->core_rsts);
>> +	if (ret)
>> +		goto err_assert_resets;
>> +
>> +	return 0;
>> +
>> +err_assert_resets:
>> +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
>> +err_disable_clks:
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
>> +
>> +	return ret;
>> +}
>> +
>> +/* Assert resets, disable app clocks */
>> +static void k1_pcie_deactivate(struct k1_pcie *k1)
> 
> k1_pcie_disable_resources()?
> 
>> +{
>> +	struct dw_pcie *pci = &k1->pci;
>> +
>> +	reset_control_bulk_assert(ARRAY_SIZE(pci->core_rsts), pci->core_rsts);
>> +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
>> +}
>> +
>> +static int k1_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 offset;
>> +	u32 mask;
>> +	u32 val;
>> +	int ret;
>> +
>> +	k1_pcie_toggle_soft_reset(k1);
>> +
>> +	ret = k1_pcie_activate(k1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_init(k1->phy);
>> +	if (ret) {
>> +		k1_pcie_deactivate(k1);
>> +
>> +		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);
>> +
>> +	/*
>> +	 * Assert fundamental reset (drive PERST# low).  Put the port in
> 
> s/port/controller

OK.

>> +	 * root complex mode, and indicate that Vaux (3.3v) is present.
>> +	 */
>> +	mask = PCIE_RC_PERST;
>> +	mask |= DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
>> +
>> +	/*
>> +	 * Write, then read back to guarantee it has reached the device
>> +	 * before we start the delay.
>> +	 */
>> +	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
>> +	regmap_set_bits(k1->pmu, offset, mask);
>> +	regmap_read(k1->pmu, offset, &val);
>> +
>> +	mdelay(PCIE_T_PVPERL_MS);
>> +
>> +	/* Deassert fundamental reset (drive PERST# high) */
>> +	regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
>> +
>> +	return 0;
>> +}
>> +
>> +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);
>> +
>> +	/* Assert fundamental reset (drive PERST# low) */
>> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			PCIE_RC_PERST);
> 
> You need assert PERST# here.

I don't understand this comment.

Setting PCIE_RC_PERST in this register drives PERST# low.

>> +	phy_exit(k1->phy);
>> +
>> +	k1_pcie_deactivate(k1);
>> +}
>> +
>> +static const struct dw_pcie_host_ops k1_pcie_host_ops = {
>> +	.init		= k1_pcie_init,
>> +	.deinit		= k1_pcie_deinit,
>> +};
>> +
>> +static bool k1_pcie_link_up(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	u32 val;
>> +
>> +	val = readl_relaxed(k1->link + K1_PHY_AHB_LINK_STS);
>> +
>> +	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
>> +}
>> +
>> +static int k1_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	u32 val;
>> +
>> +	/* Stop holding the PHY in reset, and enable link training */
>> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			   APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
>> +
>> +	/* Enable the MSI interrupt */
>> +	writel_relaxed(MSI_CTRL_INT, k1->link + INTR_ENABLE);
>> +
>> +	/* Top-level interrupt enable */
>> +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
>> +	val |= PCIE_INTERRUPT_EN;
>> +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
>> +
>> +	return 0;
>> +}
>> +
>> +static void k1_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	u32 val;
>> +
>> +	/* Disable interrupts */
>> +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
>> +	val &= ~PCIE_INTERRUPT_EN;
>> +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
>> +
>> +	writel_relaxed(0, k1->link + INTR_ENABLE);
>> +
>> +	/* Disable the link and hold the PHY in reset */
>> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			   APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
>> +}
>> +
>> +static const struct dw_pcie_ops k1_pcie_ops = {
>> +	.link_up	= k1_pcie_link_up,
>> +	.start_link	= k1_pcie_start_link,
>> +	.stop_link	= k1_pcie_stop_link,
>> +};
>> +
>> +static int k1_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct k1_pcie *k1;
>> +	int ret;
>> +
>> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>> +	if (!k1)
>> +		return -ENOMEM;
>> +
>> +	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
>> +						       SYSCON_APMU, 1,
>> +						       &k1->pmu_off);
>> +	if (IS_ERR(k1->pmu))
>> +		return dev_err_probe(dev, PTR_ERR(k1->pmu),
>> +				     "failed to lookup PMU registers\n");
>> +
>> +	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
>> +	if (!k1->link)
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "failed to map \"link\" registers\n");
>> +
>> +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "failed to get \"vpcie3v3\" supply\n");
> 
> As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
> to handle the power supplies. It is not yet handling the PERST#, but I have a
> series floating for that:
> https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/

I think that just means that I'll define a DT node compatible with
"pciclass,0604", and in that node I'll specify the vpcie3v3-supply
property.  That will cause that (pwrctrl) device to get and enable
the supply before the "real" PCIe device probes.

And once your PERST work gets merged into the PCI power control
framework, a callback will allow that to assert PERST# as needed
surrounding power transitions.  (But I won't worry about that
for now.)

Is that right?

>> +
>> +	/* Hold the PHY in reset until we start the link */
>> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			APP_HOLD_PHY_RST);
>> +
>> +	k1->phy = devm_phy_get(dev, NULL);
>> +	if (IS_ERR(k1->phy))
>> +		return dev_err_probe(dev, PTR_ERR(k1->phy),
>> +				     "failed to get PHY\n");
> 
> Once you move these properties to Root Port binding, you need to have per-Root
> Port parser. Again, you can refer the STM32 driver.

I see getting the PHY in stm32_pcie_parse_port(), but nothing
about the supply (which you mentioned in the other message).

>> +
>> +	k1->pci.dev = dev;
>> +	k1->pci.ops = &k1_pcie_ops;
>> +	dw_pcie_cap_set(&k1->pci, REQ_RES);
>> +
>> +	k1->pci.pp.ops = &k1_pcie_host_ops;
>> +	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
> 
> This driver is just using a single 'msi' vector, which can only support 32 MSIs.
> But MAX_MSI_IRQS is 256. So this looks wrong.

In dw_pcie_host_init(), if unspecified, MSI_DEF_NUM_VECTORS=32 is
used for num_vectors.  If it is specified, only if the value
exceeds MAX_MSI_IRQS=256 is an error returned.

In dw_handle_msi_irq(), "num_ctrls" is computed based on
num_vectors / MAX_MSI_IRQS_PER_CTRL=32.  A loop then
iterates over those "controllers"(?) to handle each bit
set in their corresponding register.

This seems OK.  Can you explain why you think it's wrong?

>> +
>> +	platform_set_drvdata(pdev, k1);
>> +
> 
> For setting the correct runtime PM state of the controller, you should do:
> 
> pm_runtime_set_active()
> pm_runtime_no_callbacks()
> devm_pm_runtime_enable()

OK, that's easy enough.

> This will fix the runtime PM hierarchy of PCIe chain (from host controller to
> client drivers). Otherwise, it will be broken.
Is this documented somewhere?  (It wouldn't surprise me if it
is and I just missed it.)

This driver has as its origins some vendor code, and I simply
removed the runtime PM calls.  I didn't realize something would
be broken without making pm_runtime*() calls.

Thank you for your review Mani.

					-Alex


> - Mani
> 

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 05:24:33PM -0500, Alex Elder wrote:
> On 10/26/25 11:38 AM, Manivannan Sadhasivam wrote:
> > On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
> > > Add the Device Tree binding for the PCIe root complex found on the
> > > SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
> > > PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
> > > link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
> > > typically used to support a USB 3 port.
> > > 
> > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > ---
> > > v2: - Renamed the binding, using "host controller"
> > >      - Added '>' to the description, and reworded it a bit
> > >      - Added reference to /schemas/pci/snps,dw-pcie.yaml
> > >      - Fixed and renamed the compatible string
> > >      - Renamed the PMU property, and fixed its description
> > >      - Consistently omit the period at the end of descriptions
> > >      - Renamed the "global" clock to be "phy"
> > >      - Use interrupts rather than interrupts-extended, and name the
> > >        one interrupt "msi" to make clear its purpose
> > >      - Added a vpcie3v3-supply property
> > >      - Dropped the max-link-speed property
> > >      - Changed additionalProperties to unevaluatedProperties
> > >      - Dropped the label and status property from the example
> > > 
> > >   .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
> > >   1 file changed, 156 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> > > new file mode 100644
> > > index 0000000000000..87745d49c53a1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
> > > @@ -0,0 +1,156 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SpacemiT K1 PCI Express Host Controller
> > > +
> > > +maintainers:
> > > +  - Alex Elder <elder@riscstar.com>
> > > +
> > > +description: >
> > > +  The SpacemiT K1 SoC PCIe host controller is based on the Synopsys
> > > +  DesignWare 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
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: ATU address space
> > > +      - description: PCIe configuration space
> > > +      - description: Link control registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: atu
> > > +      - const: config
> > > +      - const: link
> > > +
> > > +  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
> > > +
> > > +  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
> > > +      - description: Global reset; must be deasserted for PHY to function
> > > +
> > > +  reset-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: mstr
> > > +      - const: slv
> > > +      - const: phy
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: Interrupt used for MSIs
> > > +
> > > +  interrupt-names:
> > > +    const: msi
> > > +
> > > +  phys:
> > > +    maxItems: 1
> > > +
> > > +  vpcie3v3-supply:
> > > +    description:
> > > +      A phandle for 3.3v regulator to use for PCIe
> > 
> > Could you please move these Root Port specific properties (phy, vpcie3v3-supply)
> > to the Root Port node?
> > 
> > Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
> 
> OK, I'll try to follow what that ST binding does (and the
> matching driver).
> 
> > For handling the 'vpcie3v3-supply', you can rely on PCI_PWRCTRL_SLOT driver.
> I looked at the code under pci/pwrctrl.  But is there some other
> documentation I should be looking at for this?
> 

Sorry, nothing available atm. But I will create one, once we fix some core
issues with pwrctrl so that it becomes useable for all (more in the driver
patch).

> It looks like it involves creating a new node compatible with
> "pciclass,0604".  And that the purpose of that driver was to
> ensure certain resources are enabled before the "real" PCI
> device gets probed.
> 
> I see two arm64 DTS files using it:  x1e80100.dtsi and r8a779g0.dtsi.
> Both define this node inside the main PCIe controller node.
> 
> Will this model (with the parent pwrctrl node and child PCI
> controller node) be used for all PCI controllers from here on?
> 

The PCI controller (host bridge) node is the parent and the Root Port node
(which gets bind to pwrctrl slot driver) will be the child.

> Or are you saying this properly represents the relationship of
> the supply with the PCIe port in this SpacemiT case?
> 

We want to use this for all the new platforms and also try to convert the old
ones too gradually.

- Mani

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

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 05:24:38PM -0500, Alex Elder wrote:
> On 10/26/25 11:55 AM, Manivannan Sadhasivam wrote:
> > On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
> > > Introduce a driver for the PCIe host controller found in the SpacemiT
> > > K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> > > The driver supports three PCIe ports that operate at PCIe gen2 transfer
> > > rates (5 GT/sec).  The first port uses a combo PHY, which may be
> > > configured for use for USB 3 instead.
> > > 
> > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > ---
> > > v2: - Renamed the PCIe driver source file "pcie-spacemit-k1.c"
> > >      - Renamed the PCIe driver Kconfig option PCIE_SPACEMIT_K1; it
> > >        is now tristate rather than Boolean
> > >      - The PCIe host compatible string is now "spacemit,k1-pcie"
> > >      - Renamed the PMU syscon property to be "spacemit,apmu"
> > >      - Renamed the symbols representing the PCI vendor and device IDs
> > >        to align with <linux/pci_ids.h>
> > >      - Use PCIE_T_PVPERL_MS rather than 100 to represent a standard
> > >        delay period.
> > >      - Use platform (not dev) driver-data access functions; assignment
> > >        is done only after the private structure is initialized
> > >      - Deleted some unneeded includes in the PCIe driver.
> > >      - Dropped error checking when operating on MMIO-backed regmaps
> > >      - Added a regmap_read() call in two places, to ensure a specified
> > >        delay occurs *after* the a MMIO write has reached its target.
> > >      - Used ARRAY_SIZE() (not a local variable value) in a few spots
> > >      - Now use readl_relaxed()/writel_relaxed() when operating on
> > >        the "link" I/O memory space in the PCIe driver
> > >      - Updated a few error messages for consistency
> > >      - No longer specify suppress_bind_attrs in the PCIe driver
> > >      - Now specify PCIe driver probe type as PROBE_PREFER_ASYNCHRONOUS
> > >      - No longer use (void) cast to indicate ignored return values
> > > 
> > >   drivers/pci/controller/dwc/Kconfig            |  10 +
> > >   drivers/pci/controller/dwc/Makefile           |   1 +
> > >   drivers/pci/controller/dwc/pcie-spacemit-k1.c | 319 ++++++++++++++++++
> > >   3 files changed, 330 insertions(+)
> > >   create mode 100644 drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 349d4657393c9..ede59b34c99ba 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -509,6 +509,16 @@ config PCI_KEYSTONE_EP
> > >   	  on DesignWare hardware and therefore the driver re-uses the
> > >   	  DesignWare core functions to implement the driver.
> > > +config PCIE_SPACEMIT_K1
> > > +	tristate "SpacemiT K1 PCIe controller (host mode)"
> > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > +	depends on PCI && OF && HAS_IOMEM
> > > +	select PCIE_DW_HOST
> > > +	default ARCH_SPACEMIT
> > > +	help
> > > +	  Enables support for the PCIe controller in the K1 SoC operating
> > > +	  in host mode.
> > > +
> > >   config PCIE_VISCONTI_HOST
> > >   	bool "Toshiba Visconti PCIe controller"
> > >   	depends on ARCH_VISCONTI || COMPILE_TEST
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index 7ae28f3b0fb39..662b0a219ddc4 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> > >   obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> > >   obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
> > >   obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> > > +obj-$(CONFIG_PCIE_SPACEMIT_K1) += pcie-spacemit-k1.o
> > >   obj-$(CONFIG_PCIE_STM32_HOST) += pcie-stm32.o
> > >   obj-$(CONFIG_PCIE_STM32_EP) += pcie-stm32-ep.o
> > > diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > > new file mode 100644
> > > index 0000000000000..d58232cbb8a02
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > > @@ -0,0 +1,319 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * SpacemiT K1 PCIe host driver
> > > + *
> > > + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> > > + * Copyright (c) 2023, spacemit Corporation.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gfp.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/phy/phy.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/reset.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include "pcie-designware.h"
> > > +
> > > +#define PCI_VENDOR_ID_SPACEMIT		0x201f
> > > +#define PCI_DEVICE_ID_SPACEMIT_K1	0x0001
> > > +
> > > +/* Offsets and field definitions for link management registers */
> > > +
> > 
> > nit: drop the extra newline
> 
> OK.
> 
> > > +#define K1_PHY_AHB_IRQ_EN			0x0000
> > > +#define PCIE_INTERRUPT_EN		BIT(0)
> > > +
> > > +#define K1_PHY_AHB_LINK_STS			0x0004
> > > +#define SMLH_LINK_UP			BIT(1)
> > > +#define RDLH_LINK_UP			BIT(12)
> > > +
> > > +#define INTR_ENABLE				0x0014
> > > +#define MSI_CTRL_INT			BIT(11)
> > > +
> > > +/* Some controls require APMU regmap access */
> > > +#define SYSCON_APMU			"spacemit,apmu"
> > > +
> > > +/* Offsets and field definitions for APMU registers */
> > > +
> > 
> > here also
> 
> OK.
> 
> > > +#define PCIE_CLK_RESET_CONTROL			0x0000
> > > +#define LTSSM_EN			BIT(6)
> > > +#define PCIE_AUX_PWR_DET		BIT(9)
> > > +#define PCIE_RC_PERST			BIT(12)	/* 1: assert PERST# */
> > > +#define APP_HOLD_PHY_RST		BIT(30)
> > > +#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
> > > +
> > > +#define PCIE_CONTROL_LOGIC			0x0004
> > > +#define PCIE_SOFT_RESET			BIT(0)
> > > +
> > > +struct k1_pcie {
> > > +	struct dw_pcie pci;
> > > +	struct phy *phy;
> > > +	void __iomem *link;
> > > +	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
> > > +	u32 pmu_off;
> > > +};
> > > +
> > > +#define to_k1_pcie(dw_pcie) \
> > > +		platform_get_drvdata(to_platform_device((dw_pcie)->dev))
> > > +
> > > +static void k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
> > > +{
> > > +	u32 offset;
> > > +	u32 val;
> > > +
> > > +	/*
> > > +	 * Write, then read back to guarantee it has reached the device
> > > +	 * before we start the delay.
> > > +	 */
> > > +	offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
> > > +	regmap_set_bits(k1->pmu, offset, PCIE_SOFT_RESET);
> > > +	regmap_read(k1->pmu, offset, &val);
> > > +
> > > +	mdelay(2);
> > > +
> > > +	regmap_clear_bits(k1->pmu, offset, PCIE_SOFT_RESET);
> > > +}
> > > +
> > > +/* Enable app clocks, deassert resets */
> > > +static int k1_pcie_activate(struct k1_pcie *k1)
> > 
> > k1_pcie_enable_resources()?
> 
> OK, I'll use k1_pcie_{enable,disable}_resources().
> 
> > > +{
> > > +	struct dw_pcie *pci = &k1->pci;
> > > +	int ret;
> > > +
> > > +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->app_rsts),
> > > +					  pci->app_rsts);
> > > +	if (ret)
> > > +		goto err_disable_clks;
> > > +
> > > +	ret = reset_control_bulk_deassert(ARRAY_SIZE(pci->core_rsts),
> > > +					  pci->core_rsts);
> > > +	if (ret)
> > > +		goto err_assert_resets;
> > > +
> > > +	return 0;
> > > +
> > > +err_assert_resets:
> > > +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
> > > +err_disable_clks:
> > > +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/* Assert resets, disable app clocks */
> > > +static void k1_pcie_deactivate(struct k1_pcie *k1)
> > 
> > k1_pcie_disable_resources()?
> > 
> > > +{
> > > +	struct dw_pcie *pci = &k1->pci;
> > > +
> > > +	reset_control_bulk_assert(ARRAY_SIZE(pci->core_rsts), pci->core_rsts);
> > > +	reset_control_bulk_assert(ARRAY_SIZE(pci->app_rsts), pci->app_rsts);
> > > +	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> > > +}
> > > +
> > > +static int k1_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 offset;
> > > +	u32 mask;
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	k1_pcie_toggle_soft_reset(k1);
> > > +
> > > +	ret = k1_pcie_activate(k1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = phy_init(k1->phy);
> > > +	if (ret) {
> > > +		k1_pcie_deactivate(k1);
> > > +
> > > +		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);
> > > +
> > > +	/*
> > > +	 * Assert fundamental reset (drive PERST# low).  Put the port in
> > 
> > s/port/controller
> 
> OK.
> 
> > > +	 * root complex mode, and indicate that Vaux (3.3v) is present.
> > > +	 */
> > > +	mask = PCIE_RC_PERST;
> > > +	mask |= DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
> > > +
> > > +	/*
> > > +	 * Write, then read back to guarantee it has reached the device
> > > +	 * before we start the delay.
> > > +	 */
> > > +	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
> > > +	regmap_set_bits(k1->pmu, offset, mask);
> > > +	regmap_read(k1->pmu, offset, &val);
> > > +
> > > +	mdelay(PCIE_T_PVPERL_MS);
> > > +
> > > +	/* Deassert fundamental reset (drive PERST# high) */
> > > +	regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +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);
> > > +
> > > +	/* Assert fundamental reset (drive PERST# low) */
> > > +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > > +			PCIE_RC_PERST);
> > 
> > You need assert PERST# here.
> 
> I don't understand this comment.
> 
> Setting PCIE_RC_PERST in this register drives PERST# low.
> 

Sorry, it was a brain fade from my side. Ignore my comment.

> > > +	phy_exit(k1->phy);
> > > +
> > > +	k1_pcie_deactivate(k1);
> > > +}
> > > +
> > > +static const struct dw_pcie_host_ops k1_pcie_host_ops = {
> > > +	.init		= k1_pcie_init,
> > > +	.deinit		= k1_pcie_deinit,
> > > +};
> > > +
> > > +static bool k1_pcie_link_up(struct dw_pcie *pci)
> > > +{
> > > +	struct k1_pcie *k1 = to_k1_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	val = readl_relaxed(k1->link + K1_PHY_AHB_LINK_STS);
> > > +
> > > +	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
> > > +}
> > > +
> > > +static int k1_pcie_start_link(struct dw_pcie *pci)
> > > +{
> > > +	struct k1_pcie *k1 = to_k1_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	/* Stop holding the PHY in reset, and enable link training */
> > > +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > > +			   APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
> > > +
> > > +	/* Enable the MSI interrupt */
> > > +	writel_relaxed(MSI_CTRL_INT, k1->link + INTR_ENABLE);
> > > +
> > > +	/* Top-level interrupt enable */
> > > +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
> > > +	val |= PCIE_INTERRUPT_EN;
> > > +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void k1_pcie_stop_link(struct dw_pcie *pci)
> > > +{
> > > +	struct k1_pcie *k1 = to_k1_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	/* Disable interrupts */
> > > +	val = readl_relaxed(k1->link + K1_PHY_AHB_IRQ_EN);
> > > +	val &= ~PCIE_INTERRUPT_EN;
> > > +	writel_relaxed(val, k1->link + K1_PHY_AHB_IRQ_EN);
> > > +
> > > +	writel_relaxed(0, k1->link + INTR_ENABLE);
> > > +
> > > +	/* Disable the link and hold the PHY in reset */
> > > +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > > +			   APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
> > > +}
> > > +
> > > +static const struct dw_pcie_ops k1_pcie_ops = {
> > > +	.link_up	= k1_pcie_link_up,
> > > +	.start_link	= k1_pcie_start_link,
> > > +	.stop_link	= k1_pcie_stop_link,
> > > +};
> > > +
> > > +static int k1_pcie_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct k1_pcie *k1;
> > > +	int ret;
> > > +
> > > +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
> > > +	if (!k1)
> > > +		return -ENOMEM;
> > > +
> > > +	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
> > > +						       SYSCON_APMU, 1,
> > > +						       &k1->pmu_off);
> > > +	if (IS_ERR(k1->pmu))
> > > +		return dev_err_probe(dev, PTR_ERR(k1->pmu),
> > > +				     "failed to lookup PMU registers\n");
> > > +
> > > +	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
> > > +	if (!k1->link)
> > > +		return dev_err_probe(dev, -ENOMEM,
> > > +				     "failed to map \"link\" registers\n");
> > > +
> > > +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "failed to get \"vpcie3v3\" supply\n");
> > 
> > As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
> > to handle the power supplies. It is not yet handling the PERST#, but I have a
> > series floating for that:
> > https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/
> 
> I think that just means that I'll define a DT node compatible with
> "pciclass,0604", and in that node I'll specify the vpcie3v3-supply
> property.  That will cause that (pwrctrl) device to get and enable
> the supply before the "real" PCIe device probes.
> 

Right.

> And once your PERST work gets merged into the PCI power control
> framework, a callback will allow that to assert PERST# as needed
> surrounding power transitions.  (But I won't worry about that
> for now.)
> 

I'm still nervous to say that you should not worry about it (about not
deasserting PERST# at the right time) as it goes against the PCIe spec.
Current pwrctrl platforms supporting PERST# are working fine due to sheer luck.

So it would be better to leave the pwrctrl driver out of the equation now and
enable the supply in this driver itself. Later, once my pwrctrl rework gets
merged, I will try to switch this driver to use it.

> Is that right?
> 
> > > +
> > > +	/* Hold the PHY in reset until we start the link */
> > > +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > > +			APP_HOLD_PHY_RST);
> > > +
> > > +	k1->phy = devm_phy_get(dev, NULL);
> > > +	if (IS_ERR(k1->phy))
> > > +		return dev_err_probe(dev, PTR_ERR(k1->phy),
> > > +				     "failed to get PHY\n");
> > 
> > Once you move these properties to Root Port binding, you need to have per-Root
> > Port parser. Again, you can refer the STM32 driver.
> 
> I see getting the PHY in stm32_pcie_parse_port(), but nothing
> about the supply (which you mentioned in the other message).
> 

To conclude, you should move forward with defining the PHY and supply properties
in the Root Port node, but parse/handle them in this driver itself.

> > > +
> > > +	k1->pci.dev = dev;
> > > +	k1->pci.ops = &k1_pcie_ops;
> > > +	dw_pcie_cap_set(&k1->pci, REQ_RES);
> > > +
> > > +	k1->pci.pp.ops = &k1_pcie_host_ops;
> > > +	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
> > 
> > This driver is just using a single 'msi' vector, which can only support 32 MSIs.
> > But MAX_MSI_IRQS is 256. So this looks wrong.
> 
> In dw_pcie_host_init(), if unspecified, MSI_DEF_NUM_VECTORS=32 is
> used for num_vectors.  If it is specified, only if the value
> exceeds MAX_MSI_IRQS=256 is an error returned.
> 

Yes, because the driver trusts the glue drivers to provide the num_vectors if
they support more than 32.

> In dw_handle_msi_irq(), "num_ctrls" is computed based on
> num_vectors / MAX_MSI_IRQS_PER_CTRL=32.  A loop then
> iterates over those "controllers"(?) to handle each bit
> set in their corresponding register.
> 
> This seems OK.  Can you explain why you think it's wrong?
> 

So both 'ctrl' and 'msi' IRQs are interrelated. Each 'ctrl' can have upto 32 MSI
vectors only. If your platform supports more than 32 MSI vectors, like 256, then
the platform DT should provide 8 'msi' IRQs.

Currently the driver is not strict about this requirement. I will send a patch
to print an error message if this requirement is not satisfied.

> > > +
> > > +	platform_set_drvdata(pdev, k1);
> > > +
> > 
> > For setting the correct runtime PM state of the controller, you should do:
> > 
> > pm_runtime_set_active()
> > pm_runtime_no_callbacks()
> > devm_pm_runtime_enable()
> 
> OK, that's easy enough.
> 
> > This will fix the runtime PM hierarchy of PCIe chain (from host controller to
> > client drivers). Otherwise, it will be broken.
> Is this documented somewhere?  (It wouldn't surprise me if it
> is and I just missed it.)
> 

Sorry no. It is on my todo list. But I'm getting motivation now.

> This driver has as its origins some vendor code, and I simply
> removed the runtime PM calls.  I didn't realize something would
> be broken without making pm_runtime*() calls.
> 

It is the PM framework requirement to mark the device as 'active' to allow it to
participate in runtime PM. If you do not mark it as 'active' and 'enable' it,
the framework will not allow propagating the runtime PM changes before *this*
device. For instance, consider the generic PCI topology:

PCI controller (platform device)
	|
PCI host bridge
	|
PCI Root Port
	|
PCI endpoint device

If the runtime PM is not enabled for the PCI Root Port, then if the PCI endpoint
device runtime suspends, it will not trigger runtime suspend for the Root Port
and also for the PCI controller. Also, since the runtime PM framework doesn't
have the visibility of the devices underneath the bus (like endpoint), it may
assume that no devices (children) are currently active and may trigger runtime
suspend of the Root Port (parent) even though the endpoint device could be
'active'.

For all these reasons, it is recommended to properly reflect the runtime PM
status of the device even if the driver doesn't do anything special about it.
This is also the reason why I asked you to set pm_runtime_no_callbacks() since
this driver doesn't register any runtime PM ops.

Since this controller driver is the top of the hierarchy, you may ask what could
happen if this driver runtime PM status is not reflected correctly. Well, most
controllers have some power domain associated with them controlled by the genpd
framework. If the runtime PM framework thinks that there are no devices
connected to the bus and the controller driver also doesn't have the state
enabled, it may disable the power domain associated with it. If that happens,
the PCI controller will not work and so the devices in the hierarchy.

Hope this clarifies.

- Mani

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

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Aurelien Jarno]

Hi Alex,

On 2025-10-17 11:21, Alex Elder wrote:
> On 10/16/25 11:47 AM, Aurelien Jarno wrote:
> > Hi Alex,
> > 
> > On 2025-10-13 10:35, Alex Elder wrote:
> > > This series introduces a PHY driver and a PCIe driver to support PCIe
> > > on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
> > > Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
> > > PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
> > > one PCIe lane, and the other two ports each have two lanes.  All PCIe
> > > ports operate at 5 GT/second.
> > > 
> > > The PCIe PHYs must be configured using a value that can only be
> > > determined using the combo PHY, operating in PCIe mode.  To allow
> > > that PHY to be used for USB, the calibration step is performed by
> > > the PHY driver automatically at probe time.  Once this step is done,
> > > the PHY can be used for either PCIe or USB.
> > > 
> > > Version 2 of this series incorporates suggestions made during the
> > > review of version 1.  Specific highlights are detailed below.
> > 
> > With the issues mentioned in patch 4 fixed, this patchset works fine for
> > me. That said I had to disable ASPM by passing pcie_aspm=off on the
> > command line, as it is now enabled by default since 6.18-rc1 [1]. At
> > this stage, I am not sure if it is an issue with my NVME drive or an
> > issue with the controller.
> 
> Can you describe what symptoms you had that required you to pass
> "pcie_aspm=off" on the kernel command line?
> 
> I see these lines in my boot log related to ASPM (and added by
> the commit you link to), for both pcie1 and pcie2:
> 
>   pci 0000:01:00.0: ASPM: DT platform, enabling L0s-up L0s-dw L1 AS
> PM-L1.1 ASPM-L1.2 PCI-PM-L1.1 PCI-PM-L1.2
>   pci 0000:01:00.0: ASPM: DT platform, enabling ClockPM
> 
>   . . .
> 
>   nvme nvme0: pci function 0000:01:00.0
>   nvme 0000:01:00.0: enabling device (0000 -> 0002)
>   nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
>   nvme nvme0: 8/0/0 default/read/poll queues
>    nvme0n1: p1
> 
> My NVMe drive on pcie1 works correctly.
>   https://www.crucial.com/ssd/p3/CT1000P3SSD8
> 
>   root@bananapif3:~# df /a
>   Filesystem     1K-blocks     Used Available Use% Mounted on
>   /dev/nvme0n1p1 960302804 32063304 879385040   4% /a
>   root@bananapif3:~#

Sorry for the delay, it took me time to test some more things and 
different SSDs. First of all I still see the issue with your v3 on top 
of v6.18-rc3, which includes some fixes for ASPM support [1].

I have tried 3 different SSDs, none of them are working, but the 
symptoms are different, although all related with ASPM (pcie_aspm=off 
workarounds the issue).

With a Fox Spirit PM18 SSD (Silicon Motion, Inc. SM2263EN/SM2263XT 
controller), I do not have more than this:
[    5.196723] nvme nvme0: pci function 0000:01:00.0
[    5.198843] nvme 0000:01:00.0: enabling device (0000 -> 0002)

With a WD Blue SN570 SSD, I get this:
[    5.199513] nvme nvme0: pci function 0000:01:00.0
[    5.201653] nvme 0000:01:00.0: enabling device (0000 -> 0002)
[    5.270334] nvme nvme0: allocated 32 MiB host memory buffer (8 segments).
[    5.277624] nvme nvme0: 8/0/0 default/read/poll queues
[   19.192350] nvme nvme0: using unchecked data buffer
[   48.108400] nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
[   48.113885] nvme nvme0: Does your device have a faulty power saving mode enabled?
[   48.121346] nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug
[   48.176878] nvme0n1: I/O Cmd(0x2) @ LBA 0, 8 blocks, I/O Error (sct 0x3 / sc 0x71) 
[   48.181926] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
[   48.243670] nvme 0000:01:00.0: enabling device (0000 -> 0002)
[   48.246914] nvme nvme0: Disabling device after reset failure: -19
[   48.280495] Buffer I/O error on dev nvme0n1, logical block 0, async page read


Finally with a PNY CS1030 SSD (Phison PS5015-E15 controller), I get this:
[    5.215631] nvme nvme0: pci function 0000:01:00.0
[    5.220435] nvme 0000:01:00.0: enabling device (0000 -> 0002)
[    5.329565] nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
[   66.540485] nvme nvme0: I/O tag 28 (401c) QID 0 timeout, disable controller
[   66.585245] nvme 0000:01:00.0: probe with driver nvme failed with error -4

Note that I also tested this latest SSD on a VisionFive 2 board with exactly
the same kernel (I just moved the SSD and booted), and it works fine with ASPM
enabled (confirmed with lspci).

> I basically want to know if there's something I should do with this
> driver to address this.  (Mani, can you explain?)

I am not sure on my side how to debug that. What I know is that it is 
linked to ASPM L1, L0 works fine. In other words the SSDs work fine with 
this patch:

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 79b9651584737..1a134ec68b591 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -801,8 +801,8 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 	if (of_have_populated_dt()) {
 		if (link->aspm_support & PCIE_LINK_STATE_L0S)
 			link->aspm_default |= PCIE_LINK_STATE_L0S;
-		if (link->aspm_support & PCIE_LINK_STATE_L1)
-			link->aspm_default |= PCIE_LINK_STATE_L1;
+//		if (link->aspm_support & PCIE_LINK_STATE_L1)
+//			link->aspm_default |= PCIE_LINK_STATE_L1;
 		override = link->aspm_default & ~link->aspm_enabled;
 		if (override)
 			pci_info(pdev, "ASPM: default states%s%s\n",

I can test more things if needed, but I don't know where to start.

Regards
Aurelien

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5192d9bb0e38bf831fb93e8026e346aa017ca8

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Johannes Erdfelt]

On Tue, Oct 28, 2025, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Hi Alex,
> 
> On 2025-10-17 11:21, Alex Elder wrote:
> > On 10/16/25 11:47 AM, Aurelien Jarno wrote:
> > > Hi Alex,
> > > 
> > > On 2025-10-13 10:35, Alex Elder wrote:
> > > > This series introduces a PHY driver and a PCIe driver to support PCIe
> > > > on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
> > > > Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
> > > > PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
> > > > one PCIe lane, and the other two ports each have two lanes.  All PCIe
> > > > ports operate at 5 GT/second.
> > > > 
> > > > The PCIe PHYs must be configured using a value that can only be
> > > > determined using the combo PHY, operating in PCIe mode.  To allow
> > > > that PHY to be used for USB, the calibration step is performed by
> > > > the PHY driver automatically at probe time.  Once this step is done,
> > > > the PHY can be used for either PCIe or USB.
> > > > 
> > > > Version 2 of this series incorporates suggestions made during the
> > > > review of version 1.  Specific highlights are detailed below.
> > > 
> > > With the issues mentioned in patch 4 fixed, this patchset works fine for
> > > me. That said I had to disable ASPM by passing pcie_aspm=off on the
> > > command line, as it is now enabled by default since 6.18-rc1 [1]. At
> > > this stage, I am not sure if it is an issue with my NVME drive or an
> > > issue with the controller.
> > 
> > Can you describe what symptoms you had that required you to pass
> > "pcie_aspm=off" on the kernel command line?
> > 
> > I see these lines in my boot log related to ASPM (and added by
> > the commit you link to), for both pcie1 and pcie2:
> > 
> >   pci 0000:01:00.0: ASPM: DT platform, enabling L0s-up L0s-dw L1 AS
> > PM-L1.1 ASPM-L1.2 PCI-PM-L1.1 PCI-PM-L1.2
> >   pci 0000:01:00.0: ASPM: DT platform, enabling ClockPM
> > 
> >   . . .
> > 
> >   nvme nvme0: pci function 0000:01:00.0
> >   nvme 0000:01:00.0: enabling device (0000 -> 0002)
> >   nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
> >   nvme nvme0: 8/0/0 default/read/poll queues
> >    nvme0n1: p1
> > 
> > My NVMe drive on pcie1 works correctly.
> >   https://www.crucial.com/ssd/p3/CT1000P3SSD8
> > 
> >   root@bananapif3:~# df /a
> >   Filesystem     1K-blocks     Used Available Use% Mounted on
> >   /dev/nvme0n1p1 960302804 32063304 879385040   4% /a
> >   root@bananapif3:~#
> 
> Sorry for the delay, it took me time to test some more things and 
> different SSDs. First of all I still see the issue with your v3 on top 
> of v6.18-rc3, which includes some fixes for ASPM support [1].
> 
> I have tried 3 different SSDs, none of them are working, but the 
> symptoms are different, although all related with ASPM (pcie_aspm=off 
> workarounds the issue).
> 
> With a Fox Spirit PM18 SSD (Silicon Motion, Inc. SM2263EN/SM2263XT 
> controller), I do not have more than this:
> [    5.196723] nvme nvme0: pci function 0000:01:00.0
> [    5.198843] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> 
> With a WD Blue SN570 SSD, I get this:
> [    5.199513] nvme nvme0: pci function 0000:01:00.0
> [    5.201653] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> [    5.270334] nvme nvme0: allocated 32 MiB host memory buffer (8 segments).
> [    5.277624] nvme nvme0: 8/0/0 default/read/poll queues
> [   19.192350] nvme nvme0: using unchecked data buffer
> [   48.108400] nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
> [   48.113885] nvme nvme0: Does your device have a faulty power saving mode enabled?
> [   48.121346] nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug
> [   48.176878] nvme0n1: I/O Cmd(0x2) @ LBA 0, 8 blocks, I/O Error (sct 0x3 / sc 0x71) 
> [   48.181926] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
> [   48.243670] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> [   48.246914] nvme nvme0: Disabling device after reset failure: -19
> [   48.280495] Buffer I/O error on dev nvme0n1, logical block 0, async page read
> 
> 
> Finally with a PNY CS1030 SSD (Phison PS5015-E15 controller), I get this:
> [    5.215631] nvme nvme0: pci function 0000:01:00.0
> [    5.220435] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> [    5.329565] nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
> [   66.540485] nvme nvme0: I/O tag 28 (401c) QID 0 timeout, disable controller
> [   66.585245] nvme 0000:01:00.0: probe with driver nvme failed with error -4
> 
> Note that I also tested this latest SSD on a VisionFive 2 board with exactly
> the same kernel (I just moved the SSD and booted), and it works fine with ASPM
> enabled (confirmed with lspci).

I have been testing this patchset recently as well, but on an Orange Pi
RV2 board instead (and an extra RV2 specific patch to enable power to
the M.2 slot).

I ran into the same symptoms you had ("QID 0 timeout" after about 60
seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
seems to work fine with the "pcie_aspm=off" workaround as well.

Of note, I don't have this problem with the vendor 6.6.63 kernel.

> > I basically want to know if there's something I should do with this
> > driver to address this.  (Mani, can you explain?)
> 
> I am not sure on my side how to debug that. What I know is that it is 
> linked to ASPM L1, L0 works fine. In other words the SSDs work fine with 
> this patch:
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 79b9651584737..1a134ec68b591 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -801,8 +801,8 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>  	if (of_have_populated_dt()) {
>  		if (link->aspm_support & PCIE_LINK_STATE_L0S)
>  			link->aspm_default |= PCIE_LINK_STATE_L0S;
> -		if (link->aspm_support & PCIE_LINK_STATE_L1)
> -			link->aspm_default |= PCIE_LINK_STATE_L1;
> +//		if (link->aspm_support & PCIE_LINK_STATE_L1)
> +//			link->aspm_default |= PCIE_LINK_STATE_L1;
>  		override = link->aspm_default & ~link->aspm_enabled;
>  		if (override)
>  			pci_info(pdev, "ASPM: default states%s%s\n",
> 
> I can test more things if needed, but I don't know where to start.

I'm not a PCIe expert, but I'm more than happy to test as well.

JE

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Alex Elder]

On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> On Tue, Oct 28, 2025, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> Hi Alex,
>>
>> On 2025-10-17 11:21, Alex Elder wrote:
>>> On 10/16/25 11:47 AM, Aurelien Jarno wrote:
>>>> Hi Alex,
>>>>
>>>> On 2025-10-13 10:35, Alex Elder wrote:
>>>>> This series introduces a PHY driver and a PCIe driver to support PCIe
>>>>> on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
>>>>> Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
>>>>> PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
>>>>> one PCIe lane, and the other two ports each have two lanes.  All PCIe
>>>>> ports operate at 5 GT/second.
>>>>>
>>>>> The PCIe PHYs must be configured using a value that can only be
>>>>> determined using the combo PHY, operating in PCIe mode.  To allow
>>>>> that PHY to be used for USB, the calibration step is performed by
>>>>> the PHY driver automatically at probe time.  Once this step is done,
>>>>> the PHY can be used for either PCIe or USB.
>>>>>
>>>>> Version 2 of this series incorporates suggestions made during the
>>>>> review of version 1.  Specific highlights are detailed below.
>>>>
>>>> With the issues mentioned in patch 4 fixed, this patchset works fine for
>>>> me. That said I had to disable ASPM by passing pcie_aspm=off on the
>>>> command line, as it is now enabled by default since 6.18-rc1 [1]. At
>>>> this stage, I am not sure if it is an issue with my NVME drive or an
>>>> issue with the controller.
>>>
>>> Can you describe what symptoms you had that required you to pass
>>> "pcie_aspm=off" on the kernel command line?
>>>
>>> I see these lines in my boot log related to ASPM (and added by
>>> the commit you link to), for both pcie1 and pcie2:
>>>
>>>    pci 0000:01:00.0: ASPM: DT platform, enabling L0s-up L0s-dw L1 AS
>>> PM-L1.1 ASPM-L1.2 PCI-PM-L1.1 PCI-PM-L1.2
>>>    pci 0000:01:00.0: ASPM: DT platform, enabling ClockPM
>>>
>>>    . . .
>>>
>>>    nvme nvme0: pci function 0000:01:00.0
>>>    nvme 0000:01:00.0: enabling device (0000 -> 0002)
>>>    nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
>>>    nvme nvme0: 8/0/0 default/read/poll queues
>>>     nvme0n1: p1
>>>
>>> My NVMe drive on pcie1 works correctly.
>>>    https://www.crucial.com/ssd/p3/CT1000P3SSD8
>>>
>>>    root@bananapif3:~# df /a
>>>    Filesystem     1K-blocks     Used Available Use% Mounted on
>>>    /dev/nvme0n1p1 960302804 32063304 879385040   4% /a
>>>    root@bananapif3:~#
>>
>> Sorry for the delay, it took me time to test some more things and
>> different SSDs. First of all I still see the issue with your v3 on top
>> of v6.18-rc3, which includes some fixes for ASPM support [1].
>>
>> I have tried 3 different SSDs, none of them are working, but the
>> symptoms are different, although all related with ASPM (pcie_aspm=off
>> workarounds the issue).
>>
>> With a Fox Spirit PM18 SSD (Silicon Motion, Inc. SM2263EN/SM2263XT
>> controller), I do not have more than this:
>> [    5.196723] nvme nvme0: pci function 0000:01:00.0
>> [    5.198843] nvme 0000:01:00.0: enabling device (0000 -> 0002)
>>
>> With a WD Blue SN570 SSD, I get this:
>> [    5.199513] nvme nvme0: pci function 0000:01:00.0
>> [    5.201653] nvme 0000:01:00.0: enabling device (0000 -> 0002)
>> [    5.270334] nvme nvme0: allocated 32 MiB host memory buffer (8 segments).
>> [    5.277624] nvme nvme0: 8/0/0 default/read/poll queues
>> [   19.192350] nvme nvme0: using unchecked data buffer
>> [   48.108400] nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
>> [   48.113885] nvme nvme0: Does your device have a faulty power saving mode enabled?
>> [   48.121346] nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug
>> [   48.176878] nvme0n1: I/O Cmd(0x2) @ LBA 0, 8 blocks, I/O Error (sct 0x3 / sc 0x71)
>> [   48.181926] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
>> [   48.243670] nvme 0000:01:00.0: enabling device (0000 -> 0002)
>> [   48.246914] nvme nvme0: Disabling device after reset failure: -19
>> [   48.280495] Buffer I/O error on dev nvme0n1, logical block 0, async page read
>>
>>
>> Finally with a PNY CS1030 SSD (Phison PS5015-E15 controller), I get this:
>> [    5.215631] nvme nvme0: pci function 0000:01:00.0
>> [    5.220435] nvme 0000:01:00.0: enabling device (0000 -> 0002)
>> [    5.329565] nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
>> [   66.540485] nvme nvme0: I/O tag 28 (401c) QID 0 timeout, disable controller
>> [   66.585245] nvme 0000:01:00.0: probe with driver nvme failed with error -4
>>
>> Note that I also tested this latest SSD on a VisionFive 2 board with exactly
>> the same kernel (I just moved the SSD and booted), and it works fine with ASPM
>> enabled (confirmed with lspci).
> 
> I have been testing this patchset recently as well, but on an Orange Pi
> RV2 board instead (and an extra RV2 specific patch to enable power to
> the M.2 slot).
> 
> I ran into the same symptoms you had ("QID 0 timeout" after about 60
> seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> seems to work fine with the "pcie_aspm=off" workaround as well.

I don't see this problem, and haven't tried to reproduce it yet.

Mani told me I needed to add these lines to ensure the "runtime
PM hierarchy of PCIe chain" won't be "broken":

	pm_runtime_set_active()
	pm_runtime_no_callbacks()
	devm_pm_runtime_enable()

Just out of curiosity, could you try with those lines added
just before these assignments in k1_pcie_probe()?

	k1->pci.dev = dev;
	k1->pci.ops = &k1_pcie_ops;
	dw_pcie_cap_set(&k1->pci, REQ_RES);

I doubt it will fix what you're seeing, but at the moment I'm
working on something else.

Thanks.

					-Alex

> Of note, I don't have this problem with the vendor 6.6.63 kernel.
> 
>>> I basically want to know if there's something I should do with this
>>> driver to address this.  (Mani, can you explain?)
>>
>> I am not sure on my side how to debug that. What I know is that it is
>> linked to ASPM L1, L0 works fine. In other words the SSDs work fine with
>> this patch:
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 79b9651584737..1a134ec68b591 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -801,8 +801,8 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>>   	if (of_have_populated_dt()) {
>>   		if (link->aspm_support & PCIE_LINK_STATE_L0S)
>>   			link->aspm_default |= PCIE_LINK_STATE_L0S;
>> -		if (link->aspm_support & PCIE_LINK_STATE_L1)
>> -			link->aspm_default |= PCIE_LINK_STATE_L1;
>> +//		if (link->aspm_support & PCIE_LINK_STATE_L1)
>> +//			link->aspm_default |= PCIE_LINK_STATE_L1;
>>   		override = link->aspm_default & ~link->aspm_enabled;
>>   		if (override)
>>   			pci_info(pdev, "ASPM: default states%s%s\n",
>>
>> I can test more things if needed, but I don't know where to start.
> 
> I'm not a PCIe expert, but I'm more than happy to test as well.
> 
> JE
> 

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Johannes Erdfelt]

On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
> On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> > I have been testing this patchset recently as well, but on an Orange Pi
> > RV2 board instead (and an extra RV2 specific patch to enable power to
> > the M.2 slot).
> > 
> > I ran into the same symptoms you had ("QID 0 timeout" after about 60
> > seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> > seems to work fine with the "pcie_aspm=off" workaround as well.
> 
> I don't see this problem, and haven't tried to reproduce it yet.
> 
> Mani told me I needed to add these lines to ensure the "runtime
> PM hierarchy of PCIe chain" won't be "broken":
> 
> 	pm_runtime_set_active()
> 	pm_runtime_no_callbacks()
> 	devm_pm_runtime_enable()
> 
> Just out of curiosity, could you try with those lines added
> just before these assignments in k1_pcie_probe()?
> 
> 	k1->pci.dev = dev;
> 	k1->pci.ops = &k1_pcie_ops;
> 	dw_pcie_cap_set(&k1->pci, REQ_RES);
> 
> I doubt it will fix what you're seeing, but at the moment I'm
> working on something else.

Unfortunately there is no difference with the runtime PM hierarchy
additions.

JE

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Alex Elder]

On 10/28/25 3:48 PM, Johannes Erdfelt wrote:
> On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
>> On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
>>> I have been testing this patchset recently as well, but on an Orange Pi
>>> RV2 board instead (and an extra RV2 specific patch to enable power to
>>> the M.2 slot).
>>>
>>> I ran into the same symptoms you had ("QID 0 timeout" after about 60
>>> seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
>>> seems to work fine with the "pcie_aspm=off" workaround as well.
>>
>> I don't see this problem, and haven't tried to reproduce it yet.
>>
>> Mani told me I needed to add these lines to ensure the "runtime
>> PM hierarchy of PCIe chain" won't be "broken":
>>
>> 	pm_runtime_set_active()
>> 	pm_runtime_no_callbacks()
>> 	devm_pm_runtime_enable()
>>
>> Just out of curiosity, could you try with those lines added
>> just before these assignments in k1_pcie_probe()?
>>
>> 	k1->pci.dev = dev;
>> 	k1->pci.ops = &k1_pcie_ops;
>> 	dw_pcie_cap_set(&k1->pci, REQ_RES);
>>
>> I doubt it will fix what you're seeing, but at the moment I'm
>> working on something else.
> 
> Unfortunately there is no difference with the runtime PM hierarchy
> additions.
> 
> JE

Thank you very much for testing.  I'll try to learn more
about this in the next day or so and will resolve it if
possible before I send the next version of this code.

					-Alex

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Aurelien Jarno]

On 2025-10-28 14:10, Alex Elder wrote:
> On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> > On Tue, Oct 28, 2025, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > > Hi Alex,
> > > 
> > > On 2025-10-17 11:21, Alex Elder wrote:
> > > > On 10/16/25 11:47 AM, Aurelien Jarno wrote:
> > > > > Hi Alex,
> > > > > 
> > > > > On 2025-10-13 10:35, Alex Elder wrote:
> > > > > > This series introduces a PHY driver and a PCIe driver to support PCIe
> > > > > > on the SpacemiT K1 SoC.  The PCIe implementation is derived from a
> > > > > > Synopsys DesignWare PCIe IP.  The PHY driver supports one combination
> > > > > > PCIe/USB PHY as well as two PCIe-only PHYs.  The combo PHY port uses
> > > > > > one PCIe lane, and the other two ports each have two lanes.  All PCIe
> > > > > > ports operate at 5 GT/second.
> > > > > > 
> > > > > > The PCIe PHYs must be configured using a value that can only be
> > > > > > determined using the combo PHY, operating in PCIe mode.  To allow
> > > > > > that PHY to be used for USB, the calibration step is performed by
> > > > > > the PHY driver automatically at probe time.  Once this step is done,
> > > > > > the PHY can be used for either PCIe or USB.
> > > > > > 
> > > > > > Version 2 of this series incorporates suggestions made during the
> > > > > > review of version 1.  Specific highlights are detailed below.
> > > > > 
> > > > > With the issues mentioned in patch 4 fixed, this patchset works fine for
> > > > > me. That said I had to disable ASPM by passing pcie_aspm=off on the
> > > > > command line, as it is now enabled by default since 6.18-rc1 [1]. At
> > > > > this stage, I am not sure if it is an issue with my NVME drive or an
> > > > > issue with the controller.
> > > > 
> > > > Can you describe what symptoms you had that required you to pass
> > > > "pcie_aspm=off" on the kernel command line?
> > > > 
> > > > I see these lines in my boot log related to ASPM (and added by
> > > > the commit you link to), for both pcie1 and pcie2:
> > > > 
> > > >    pci 0000:01:00.0: ASPM: DT platform, enabling L0s-up L0s-dw L1 AS
> > > > PM-L1.1 ASPM-L1.2 PCI-PM-L1.1 PCI-PM-L1.2
> > > >    pci 0000:01:00.0: ASPM: DT platform, enabling ClockPM
> > > > 
> > > >    . . .
> > > > 
> > > >    nvme nvme0: pci function 0000:01:00.0
> > > >    nvme 0000:01:00.0: enabling device (0000 -> 0002)
> > > >    nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
> > > >    nvme nvme0: 8/0/0 default/read/poll queues
> > > >     nvme0n1: p1
> > > > 
> > > > My NVMe drive on pcie1 works correctly.
> > > >    https://www.crucial.com/ssd/p3/CT1000P3SSD8
> > > > 
> > > >    root@bananapif3:~# df /a
> > > >    Filesystem     1K-blocks     Used Available Use% Mounted on
> > > >    /dev/nvme0n1p1 960302804 32063304 879385040   4% /a
> > > >    root@bananapif3:~#
> > > 
> > > Sorry for the delay, it took me time to test some more things and
> > > different SSDs. First of all I still see the issue with your v3 on top
> > > of v6.18-rc3, which includes some fixes for ASPM support [1].
> > > 
> > > I have tried 3 different SSDs, none of them are working, but the
> > > symptoms are different, although all related with ASPM (pcie_aspm=off
> > > workarounds the issue).
> > > 
> > > With a Fox Spirit PM18 SSD (Silicon Motion, Inc. SM2263EN/SM2263XT
> > > controller), I do not have more than this:
> > > [    5.196723] nvme nvme0: pci function 0000:01:00.0
> > > [    5.198843] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> > > 
> > > With a WD Blue SN570 SSD, I get this:
> > > [    5.199513] nvme nvme0: pci function 0000:01:00.0
> > > [    5.201653] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> > > [    5.270334] nvme nvme0: allocated 32 MiB host memory buffer (8 segments).
> > > [    5.277624] nvme nvme0: 8/0/0 default/read/poll queues
> > > [   19.192350] nvme nvme0: using unchecked data buffer
> > > [   48.108400] nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
> > > [   48.113885] nvme nvme0: Does your device have a faulty power saving mode enabled?
> > > [   48.121346] nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug
> > > [   48.176878] nvme0n1: I/O Cmd(0x2) @ LBA 0, 8 blocks, I/O Error (sct 0x3 / sc 0x71)
> > > [   48.181926] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
> > > [   48.243670] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> > > [   48.246914] nvme nvme0: Disabling device after reset failure: -19
> > > [   48.280495] Buffer I/O error on dev nvme0n1, logical block 0, async page read
> > > 
> > > 
> > > Finally with a PNY CS1030 SSD (Phison PS5015-E15 controller), I get this:
> > > [    5.215631] nvme nvme0: pci function 0000:01:00.0
> > > [    5.220435] nvme 0000:01:00.0: enabling device (0000 -> 0002)
> > > [    5.329565] nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
> > > [   66.540485] nvme nvme0: I/O tag 28 (401c) QID 0 timeout, disable controller
> > > [   66.585245] nvme 0000:01:00.0: probe with driver nvme failed with error -4
> > > 
> > > Note that I also tested this latest SSD on a VisionFive 2 board with exactly
> > > the same kernel (I just moved the SSD and booted), and it works fine with ASPM
> > > enabled (confirmed with lspci).
> > 
> > I have been testing this patchset recently as well, but on an Orange Pi
> > RV2 board instead (and an extra RV2 specific patch to enable power to
> > the M.2 slot).
> > 
> > I ran into the same symptoms you had ("QID 0 timeout" after about 60
> > seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> > seems to work fine with the "pcie_aspm=off" workaround as well.
> 
> I don't see this problem, and haven't tried to reproduce it yet.
> 
> Mani told me I needed to add these lines to ensure the "runtime
> PM hierarchy of PCIe chain" won't be "broken":
> 
> 	pm_runtime_set_active()
> 	pm_runtime_no_callbacks()
> 	devm_pm_runtime_enable()
> 
> Just out of curiosity, could you try with those lines added
> just before these assignments in k1_pcie_probe()?
> 
> 	k1->pci.dev = dev;
> 	k1->pci.ops = &k1_pcie_ops;
> 	dw_pcie_cap_set(&k1->pci, REQ_RES);
> 
> I doubt it will fix what you're seeing, but at the moment I'm
> working on something else.
> 

Thanks for your fast answer. I have just tried this patch:

--- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
+++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
@@ -271,6 +271,16 @@ static int k1_pcie_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(k1->phy),
 				     "failed to get PHY\n");
 
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to activate runtime PM\n");
+
+	pm_runtime_no_callbacks(dev);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
 	k1->pci.dev = dev;
 	k1->pci.ops = &k1_pcie_ops;
 	dw_pcie_cap_set(&k1->pci, REQ_RES);

Unfortunately this doesn't fix the issue. On the positive side, things 
still work with it and pcie_aspm=off.

Regards
AUrelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v2 3/7] dt-bindings: pci: spacemit: introduce PCIe host controller

[Alex Elder]

On 10/28/25 12:58 AM, Manivannan Sadhasivam wrote:
> On Mon, Oct 27, 2025 at 05:24:33PM -0500, Alex Elder wrote:
>> On 10/26/25 11:38 AM, Manivannan Sadhasivam wrote:
>>> On Mon, Oct 13, 2025 at 10:35:20AM -0500, Alex Elder wrote:
>>>> Add the Device Tree binding for the PCIe root complex found on the
>>>> SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
>>>> PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
>>>> link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
>>>> typically used to support a USB 3 port.
>>>>
>>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>>> ---
>>>> v2: - Renamed the binding, using "host controller"
>>>>       - Added '>' to the description, and reworded it a bit
>>>>       - Added reference to /schemas/pci/snps,dw-pcie.yaml
>>>>       - Fixed and renamed the compatible string
>>>>       - Renamed the PMU property, and fixed its description
>>>>       - Consistently omit the period at the end of descriptions
>>>>       - Renamed the "global" clock to be "phy"
>>>>       - Use interrupts rather than interrupts-extended, and name the
>>>>         one interrupt "msi" to make clear its purpose
>>>>       - Added a vpcie3v3-supply property
>>>>       - Dropped the max-link-speed property
>>>>       - Changed additionalProperties to unevaluatedProperties
>>>>       - Dropped the label and status property from the example
>>>>
>>>>    .../bindings/pci/spacemit,k1-pcie-host.yaml   | 156 ++++++++++++++++++
>>>>    1 file changed, 156 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>>> new file mode 100644
>>>> index 0000000000000..87745d49c53a1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml
>>>> @@ -0,0 +1,156 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/pci/spacemit,k1-pcie-host.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: SpacemiT K1 PCI Express Host Controller

. . .

>>>> +  interrupt-names:
>>>> +    const: msi
>>>> +
>>>> +  phys:
>>>> +    maxItems: 1
>>>> +
>>>> +  vpcie3v3-supply:
>>>> +    description:
>>>> +      A phandle for 3.3v regulator to use for PCIe
>>>
>>> Could you please move these Root Port specific properties (phy, vpcie3v3-supply)
>>> to the Root Port node?
>>>
>>> Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
>>
>> OK, I'll try to follow what that ST binding does (and the
>> matching driver).
>>
>>> For handling the 'vpcie3v3-supply', you can rely on PCI_PWRCTRL_SLOT driver.
>> I looked at the code under pci/pwrctrl.  But is there some other
>> documentation I should be looking at for this?
>>
> 
> Sorry, nothing available atm. But I will create one, once we fix some core
> issues with pwrctrl so that it becomes useable for all (more in the driver
> patch).

Sounds good, I think it's necessary.  I might not get it completely
right on the next try but I trust you'll help me understand what I
need to do.

>> It looks like it involves creating a new node compatible with
>> "pciclass,0604".  And that the purpose of that driver was to
>> ensure certain resources are enabled before the "real" PCI
>> device gets probed.
>>
>> I see two arm64 DTS files using it:  x1e80100.dtsi and r8a779g0.dtsi.
>> Both define this node inside the main PCIe controller node.
>>
>> Will this model (with the parent pwrctrl node and child PCI
>> controller node) be used for all PCI controllers from here on?
>>
> 
> The PCI controller (host bridge) node is the parent and the Root Port node
> (which gets bind to pwrctrl slot driver) will be the child.

That makes sense to me.

>> Or are you saying this properly represents the relationship of
>> the supply with the PCIe port in this SpacemiT case?
>>
> 
> We want to use this for all the new platforms and also try to convert the old
> ones too gradually.

OK, understood.

Thank you.

					-Alex

> 
> - Mani
> 

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Alex Elder]

On 10/28/25 2:06 AM, Manivannan Sadhasivam wrote:
> On Mon, Oct 27, 2025 at 05:24:38PM -0500, Alex Elder wrote:
>> On 10/26/25 11:55 AM, Manivannan Sadhasivam wrote:
>>> On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
>>>> Introduce a driver for the PCIe host controller found in the SpacemiT
>>>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>>>> The driver supports three PCIe ports that operate at PCIe gen2 transfer
>>>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>>>> configured for use for USB 3 instead.
>>>>
>>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>>> ---

. . .

>>>> +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
>>>> +	if (ret)
>>>> +		return dev_err_probe(dev, ret,
>>>> +				     "failed to get \"vpcie3v3\" supply\n");
>>>
>>> As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
>>> to handle the power supplies. It is not yet handling the PERST#, but I have a
>>> series floating for that:
>>> https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/
>>
>> I think that just means that I'll define a DT node compatible with
>> "pciclass,0604", and in that node I'll specify the vpcie3v3-supply
>> property.  That will cause that (pwrctrl) device to get and enable
>> the supply before the "real" PCIe device probes.
>>
> 
> Right.
> 
>> And once your PERST work gets merged into the PCI power control
>> framework, a callback will allow that to assert PERST# as needed
>> surrounding power transitions.  (But I won't worry about that
>> for now.)
>>
> 
> I'm still nervous to say that you should not worry about it (about not
> deasserting PERST# at the right time) as it goes against the PCIe spec.
> Current pwrctrl platforms supporting PERST# are working fine due to sheer luck.
> 
> So it would be better to leave the pwrctrl driver out of the equation now and
> enable the supply in this driver itself. Later, once my pwrctrl rework gets
> merged, I will try to switch this driver to use it.

As I understand it, PERST# should be only be deasserted after
all power rails are known to be stable.

This driver enables the regulator during probe, shortly
before calling dw_pcie_host_init().  That function calls
back to k1_pcie_init(), which enables clocks, deasserts
resets, and initializes the PHY before it changes the
PERST# state.

By "changing the PERST# state" I mean it is asserted
(driven low), then deasserted after 100 milliseconds
(PCIE_T_PVPERL_MS).

I have two questions on this:
- You say the PCI spec talks about the "right time" to
   deassert PERST# (relative to power).  Is that at all
   related to PCIE_T_PVPERL_MS?
- I notice that PERST# is in a deasserted state at the
   time I assert it in this sequence.  Do you see any
   reason I should assert it early as an initialization
   step, or is asserting it and holding it there for
   100 msec sufficient?

>> Is that right?
>>
>>>> +
>>>> +	/* Hold the PHY in reset until we start the link */
>>>> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>>>> +			APP_HOLD_PHY_RST);
>>>> +
>>>> +	k1->phy = devm_phy_get(dev, NULL);
>>>> +	if (IS_ERR(k1->phy))
>>>> +		return dev_err_probe(dev, PTR_ERR(k1->phy),
>>>> +				     "failed to get PHY\n");
>>>
>>> Once you move these properties to Root Port binding, you need to have per-Root
>>> Port parser. Again, you can refer the STM32 driver.
>>
>> I see getting the PHY in stm32_pcie_parse_port(), but nothing
>> about the supply (which you mentioned in the other message).
>>
> 
> To conclude, you should move forward with defining the PHY and supply properties
> in the Root Port node, but parse/handle them in this driver itself.

Got it.

>>>> +
>>>> +	k1->pci.dev = dev;
>>>> +	k1->pci.ops = &k1_pcie_ops;
>>>> +	dw_pcie_cap_set(&k1->pci, REQ_RES);
>>>> +
>>>> +	k1->pci.pp.ops = &k1_pcie_host_ops;
>>>> +	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
>>>
>>> This driver is just using a single 'msi' vector, which can only support 32 MSIs.
>>> But MAX_MSI_IRQS is 256. So this looks wrong.
>>
>> In dw_pcie_host_init(), if unspecified, MSI_DEF_NUM_VECTORS=32 is
>> used for num_vectors.  If it is specified, only if the value
>> exceeds MAX_MSI_IRQS=256 is an error returned.
>>
> 
> Yes, because the driver trusts the glue drivers to provide the num_vectors if
> they support more than 32.
> 
>> In dw_handle_msi_irq(), "num_ctrls" is computed based on
>> num_vectors / MAX_MSI_IRQS_PER_CTRL=32.  A loop then
>> iterates over those "controllers"(?) to handle each bit
>> set in their corresponding register.
>>
>> This seems OK.  Can you explain why you think it's wrong?
>>
> 
> So both 'ctrl' and 'msi' IRQs are interrelated. Each 'ctrl' can have upto 32 MSI
> vectors only. If your platform supports more than 32 MSI vectors, like 256, then
> the platform DT should provide 8 'msi' IRQs.

I have asked SpacemiT about this, specifically whether there
are additional interrupts (I don't think there are), or if
not that, additional registers to support MSI 32+ (see
below).  In their downstream driver they handle interrupts
differently.  I suspect num_vectors needs to be set to 32
(or I'll leave it unset and take the default).


In the DesignWare driver, there are up to 8 "ctrls", and each
ctrl has 32 bit positions representing 32 MSI vectors.  Each
can have an msi_irq defined.  An msi_irq is always set up for
ctrl 0.

For any ctrl with an msi_irq assigned, dw_pcie_msi_host_init()
sets its interrupt handler to dw_chained_msi_isr(), which just
calls dw_handle_msi_irq().

The way dw_handle_msi_irq() works, a single ctrl apparently can
handle up to 256 MSI vectors, as long as the block of 3 registers
that manage the ctrl (ENABLE, MASK, and STATUS presumably) are
consecutive in I/O memory for consecutive ctrls.


I looked for other examples.  I see that "pcie-fu740.c", which
supports compatible "sifive,fu740-pcie", sets num_vectors to
MAX_MSI_IRQS, but "fu740-c000.dtsi" defines just one "msi"
interrupt.  And "pci-dra7xx.c" seems to do something similar,
and maybe "pcie-rcar-gen4.c" too.

> Currently the driver is not strict about this requirement. I will send a patch
> to print an error message if this requirement is not satisfied.
> 
>>>> +
>>>> +	platform_set_drvdata(pdev, k1);
>>>> +
>>>
>>> For setting the correct runtime PM state of the controller, you should do:
>>>
>>> pm_runtime_set_active()
>>> pm_runtime_no_callbacks()
>>> devm_pm_runtime_enable()
>>
>> OK, that's easy enough.
>>
>>> This will fix the runtime PM hierarchy of PCIe chain (from host controller to
>>> client drivers). Otherwise, it will be broken.
>> Is this documented somewhere?  (It wouldn't surprise me if it
>> is and I just missed it.)
>>
> 
> Sorry no. It is on my todo list. But I'm getting motivation now.
> 
>> This driver has as its origins some vendor code, and I simply
>> removed the runtime PM calls.  I didn't realize something would
>> be broken without making pm_runtime*() calls.
>>
> 
> It is the PM framework requirement to mark the device as 'active' to allow it to
> participate in runtime PM. If you do not mark it as 'active' and 'enable' it,
> the framework will not allow propagating the runtime PM changes before *this*
> device. For instance, consider the generic PCI topology:
> 
> PCI controller (platform device)
> 	|
> PCI host bridge
> 	|
> PCI Root Port
> 	|
> PCI endpoint device
> 
> If the runtime PM is not enabled for the PCI Root Port, then if the PCI endpoint
> device runtime suspends, it will not trigger runtime suspend for the Root Port
> and also for the PCI controller. Also, since the runtime PM framework doesn't
> have the visibility of the devices underneath the bus (like endpoint), it may
> assume that no devices (children) are currently active and may trigger runtime
> suspend of the Root Port (parent) even though the endpoint device could be
> 'active'.

So this basically marks this controller as a pass-through device that
doesn't itself change state for runtime PM, but still communicates that
somewhere at or below it there might be devices that do participate.
  > For all these reasons, it is recommended to properly reflect the 
runtime PM
> status of the device even if the driver doesn't do anything special about it.
> This is also the reason why I asked you to set pm_runtime_no_callbacks() since
> this driver doesn't register any runtime PM ops.
OK, I think understand now.

> Since this controller driver is the top of the hierarchy, you may ask what could
> happen if this driver runtime PM status is not reflected correctly. Well, most
> controllers have some power domain associated with them controlled by the genpd
> framework. If the runtime PM framework thinks that there are no devices
> connected to the bus and the controller driver also doesn't have the state
> enabled, it may disable the power domain associated with it. If that happens,
> the PCI controller will not work and so the devices in the hierarchy.
> 
> Hope this clarifies.

Yes, it helps a lot.  Thank you.

					-Alex

> - Mani
> 

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Manivannan Sadhasivam]

+ Aurelien

On Tue, Oct 28, 2025 at 01:48:32PM -0700, Johannes Erdfelt wrote:
> On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
> > On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> > > I have been testing this patchset recently as well, but on an Orange Pi
> > > RV2 board instead (and an extra RV2 specific patch to enable power to
> > > the M.2 slot).
> > > 
> > > I ran into the same symptoms you had ("QID 0 timeout" after about 60
> > > seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> > > seems to work fine with the "pcie_aspm=off" workaround as well.
> > 
> > I don't see this problem, and haven't tried to reproduce it yet.
> > 
> > Mani told me I needed to add these lines to ensure the "runtime
> > PM hierarchy of PCIe chain" won't be "broken":
> > 
> > 	pm_runtime_set_active()
> > 	pm_runtime_no_callbacks()
> > 	devm_pm_runtime_enable()
> > 
> > Just out of curiosity, could you try with those lines added
> > just before these assignments in k1_pcie_probe()?
> > 
> > 	k1->pci.dev = dev;
> > 	k1->pci.ops = &k1_pcie_ops;
> > 	dw_pcie_cap_set(&k1->pci, REQ_RES);
> > 
> > I doubt it will fix what you're seeing, but at the moment I'm
> > working on something else.
> 
> Unfortunately there is no difference with the runtime PM hierarchy
> additions.
> 

These are not supposed to fix the issues you were facing. I discussed with Alex
offline and figured out that L1 works fine on his BPI-F3 board with a NVMe SSD.

And I believe, Aurelien is also using that same board, but with different
SSDs. But what is puzzling me is, L1 is breaking Aurelien's setup with 3 SSDs
from different vendors. It apparently works fine on Alex's setup. So it somehow
confirms that Root Port supports and behaves correctly with L1. But at the same
time, I cannot just say without evidence that L1 is broken on all these SSDs
that you and Aurelien tested with.

So until that is figured out, I've asked Alex to disable L1 CAP in the
controller driver. So in the next version of this series, your SSDs should work
out of the box.

- Mani

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

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Aurelien Jarno]

Hi Mani,

On 2025-10-30 22:11, Manivannan Sadhasivam wrote:
> + Aurelien
> 
> On Tue, Oct 28, 2025 at 01:48:32PM -0700, Johannes Erdfelt wrote:
> > On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
> > > On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> > > > I have been testing this patchset recently as well, but on an Orange Pi
> > > > RV2 board instead (and an extra RV2 specific patch to enable power to
> > > > the M.2 slot).
> > > > 
> > > > I ran into the same symptoms you had ("QID 0 timeout" after about 60
> > > > seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> > > > seems to work fine with the "pcie_aspm=off" workaround as well.
> > > 
> > > I don't see this problem, and haven't tried to reproduce it yet.
> > > 
> > > Mani told me I needed to add these lines to ensure the "runtime
> > > PM hierarchy of PCIe chain" won't be "broken":
> > > 
> > > 	pm_runtime_set_active()
> > > 	pm_runtime_no_callbacks()
> > > 	devm_pm_runtime_enable()
> > > 
> > > Just out of curiosity, could you try with those lines added
> > > just before these assignments in k1_pcie_probe()?
> > > 
> > > 	k1->pci.dev = dev;
> > > 	k1->pci.ops = &k1_pcie_ops;
> > > 	dw_pcie_cap_set(&k1->pci, REQ_RES);
> > > 
> > > I doubt it will fix what you're seeing, but at the moment I'm
> > > working on something else.
> > 
> > Unfortunately there is no difference with the runtime PM hierarchy
> > additions.
> > 
> 
> These are not supposed to fix the issues you were facing. I discussed with Alex
> offline and figured out that L1 works fine on his BPI-F3 board with a NVMe SSD.
> 
> And I believe, Aurelien is also using that same board, but with different
> SSDs. But what is puzzling me is, L1 is breaking Aurelien's setup with 3 SSDs
> from different vendors. It apparently works fine on Alex's setup. So it somehow
> confirms that Root Port supports and behaves correctly with L1. But at the same
> time, I cannot just say without evidence that L1 is broken on all these SSDs
> that you and Aurelien tested with.

It could be that we have different revision of the BPI-F3 board, it's 
not impossible that I got an early-ish version. That said I just 
visually checked the PCB against the schematics, and the devices on the 
CLKREQN line appear to be installed.

If someone has contacts to check what changes have been done between the 
different board revision, that could help. Or same if there are 
different revisions of the SpacemiT K1 chip.

> So until that is figured out, I've asked Alex to disable L1 CAP in the
> controller driver. So in the next version of this series, your SSDs should work
> out of the box.

Thanks, that sounds good.

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Manivannan Sadhasivam]

On Wed, Oct 29, 2025 at 07:10:10PM -0500, Alex Elder wrote:
> On 10/28/25 2:06 AM, Manivannan Sadhasivam wrote:
> > On Mon, Oct 27, 2025 at 05:24:38PM -0500, Alex Elder wrote:
> > > On 10/26/25 11:55 AM, Manivannan Sadhasivam wrote:
> > > > On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
> > > > > Introduce a driver for the PCIe host controller found in the SpacemiT
> > > > > K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> > > > > The driver supports three PCIe ports that operate at PCIe gen2 transfer
> > > > > rates (5 GT/sec).  The first port uses a combo PHY, which may be
> > > > > configured for use for USB 3 instead.
> > > > > 
> > > > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > > > ---
> 
> . . .
> 
> > > > > +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
> > > > > +	if (ret)
> > > > > +		return dev_err_probe(dev, ret,
> > > > > +				     "failed to get \"vpcie3v3\" supply\n");
> > > > 
> > > > As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
> > > > to handle the power supplies. It is not yet handling the PERST#, but I have a
> > > > series floating for that:
> > > > https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/
> > > 
> > > I think that just means that I'll define a DT node compatible with
> > > "pciclass,0604", and in that node I'll specify the vpcie3v3-supply
> > > property.  That will cause that (pwrctrl) device to get and enable
> > > the supply before the "real" PCIe device probes.
> > > 
> > 
> > Right.
> > 
> > > And once your PERST work gets merged into the PCI power control
> > > framework, a callback will allow that to assert PERST# as needed
> > > surrounding power transitions.  (But I won't worry about that
> > > for now.)
> > > 
> > 
> > I'm still nervous to say that you should not worry about it (about not
> > deasserting PERST# at the right time) as it goes against the PCIe spec.
> > Current pwrctrl platforms supporting PERST# are working fine due to sheer luck.
> > 
> > So it would be better to leave the pwrctrl driver out of the equation now and
> > enable the supply in this driver itself. Later, once my pwrctrl rework gets
> > merged, I will try to switch this driver to use it.
> 
> As I understand it, PERST# should be only be deasserted after
> all power rails are known to be stable.
> 

Yes

> This driver enables the regulator during probe, shortly
> before calling dw_pcie_host_init().  That function calls
> back to k1_pcie_init(), which enables clocks, deasserts
> resets, and initializes the PHY before it changes the
> PERST# state.
> 
> By "changing the PERST# state" I mean it is asserted
> (driven low), then deasserted after 100 milliseconds
> (PCIE_T_PVPERL_MS).
> 
> I have two questions on this:
> - You say the PCI spec talks about the "right time" to
>   deassert PERST# (relative to power).  Is that at all
>   related to PCIE_T_PVPERL_MS?

The PCI CEM spec says that PERST# should be deasserted atleast 100ms after the
power becomes stable. But with the current pwrctrl design, the host controller
deasserts the PERST# even before the pwrctrl probe. So this is in violation of
the spec. But depending on the endpoint device design, this might not cause any
issue as PERST# is a level triggered signal. So once the endpoint boots up, it
will see the PERST# deassert and will start working. I'm not justifying the
current design, but just mentioning that you might not see any issue.

That being said, we are going to submit a series that reworks pwrctrl framework
such that each controller can call an API to probe pwrctrl drivers. This way,
host controller driver can make sure that the device will get powered ON before
it deasserts the PERST#.

> - I notice that PERST# is in a deasserted state at the
>   time I assert it in this sequence.  Do you see any
>   reason I should assert it early as an initialization
>   step, or is asserting it and holding it there for
>   100 msec sufficient?
> 

You should assert PERST# before doing any controller initialization sequence as
that may affect the endpoint. Once PERST# is asserted, it will cause the
endpoint to 'reset'. So you do your initialization sequence and deassert it once
done.

> > > Is that right?
> > > 
> > > > > +
> > > > > +	/* Hold the PHY in reset until we start the link */
> > > > > +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > > > > +			APP_HOLD_PHY_RST);
> > > > > +
> > > > > +	k1->phy = devm_phy_get(dev, NULL);
> > > > > +	if (IS_ERR(k1->phy))
> > > > > +		return dev_err_probe(dev, PTR_ERR(k1->phy),
> > > > > +				     "failed to get PHY\n");
> > > > 
> > > > Once you move these properties to Root Port binding, you need to have per-Root
> > > > Port parser. Again, you can refer the STM32 driver.
> > > 
> > > I see getting the PHY in stm32_pcie_parse_port(), but nothing
> > > about the supply (which you mentioned in the other message).
> > > 
> > 
> > To conclude, you should move forward with defining the PHY and supply properties
> > in the Root Port node, but parse/handle them in this driver itself.
> 
> Got it.
> 
> > > > > +
> > > > > +	k1->pci.dev = dev;
> > > > > +	k1->pci.ops = &k1_pcie_ops;
> > > > > +	dw_pcie_cap_set(&k1->pci, REQ_RES);
> > > > > +
> > > > > +	k1->pci.pp.ops = &k1_pcie_host_ops;
> > > > > +	k1->pci.pp.num_vectors = MAX_MSI_IRQS;
> > > > 
> > > > This driver is just using a single 'msi' vector, which can only support 32 MSIs.
> > > > But MAX_MSI_IRQS is 256. So this looks wrong.
> > > 
> > > In dw_pcie_host_init(), if unspecified, MSI_DEF_NUM_VECTORS=32 is
> > > used for num_vectors.  If it is specified, only if the value
> > > exceeds MAX_MSI_IRQS=256 is an error returned.
> > > 
> > 
> > Yes, because the driver trusts the glue drivers to provide the num_vectors if
> > they support more than 32.
> > 
> > > In dw_handle_msi_irq(), "num_ctrls" is computed based on
> > > num_vectors / MAX_MSI_IRQS_PER_CTRL=32.  A loop then
> > > iterates over those "controllers"(?) to handle each bit
> > > set in their corresponding register.
> > > 
> > > This seems OK.  Can you explain why you think it's wrong?
> > > 
> > 
> > So both 'ctrl' and 'msi' IRQs are interrelated. Each 'ctrl' can have upto 32 MSI
> > vectors only. If your platform supports more than 32 MSI vectors, like 256, then
> > the platform DT should provide 8 'msi' IRQs.
> 
> I have asked SpacemiT about this, specifically whether there
> are additional interrupts (I don't think there are), or if
> not that, additional registers to support MSI 32+ (see
> below).  In their downstream driver they handle interrupts
> differently.  I suspect num_vectors needs to be set to 32
> (or I'll leave it unset and take the default).
> 
> 
> In the DesignWare driver, there are up to 8 "ctrls", and each
> ctrl has 32 bit positions representing 32 MSI vectors.  Each
> can have an msi_irq defined.  An msi_irq is always set up for
> ctrl 0.
> 
> For any ctrl with an msi_irq assigned, dw_pcie_msi_host_init()
> sets its interrupt handler to dw_chained_msi_isr(), which just
> calls dw_handle_msi_irq().
> 
> The way dw_handle_msi_irq() works, a single ctrl apparently can
> handle up to 256 MSI vectors, as long as the block of 3 registers
> that manage the ctrl (ENABLE, MASK, and STATUS presumably) are
> consecutive in I/O memory for consecutive ctrls.
> 

I'm not sure how you came up with this observation. dw_handle_msi_irq() loops
over the 'status' using find_next_bit() of size MAX_MSI_IRQS_PER_CTRL, which is
32. So I don't see how a single ctrl can handle up to 256 vectors.

> 
> I looked for other examples.  I see that "pcie-fu740.c", which
> supports compatible "sifive,fu740-pcie", sets num_vectors to
> MAX_MSI_IRQS, but "fu740-c000.dtsi" defines just one "msi"
> interrupt.  And "pci-dra7xx.c" seems to do something similar,
> and maybe "pcie-rcar-gen4.c" too.
> 

Yes. But I think those are mistakes. I will submit patches to fix them.

> > Currently the driver is not strict about this requirement. I will send a patch
> > to print an error message if this requirement is not satisfied.
> > 
> > > > > +
> > > > > +	platform_set_drvdata(pdev, k1);
> > > > > +
> > > > 
> > > > For setting the correct runtime PM state of the controller, you should do:
> > > > 
> > > > pm_runtime_set_active()
> > > > pm_runtime_no_callbacks()
> > > > devm_pm_runtime_enable()
> > > 
> > > OK, that's easy enough.
> > > 
> > > > This will fix the runtime PM hierarchy of PCIe chain (from host controller to
> > > > client drivers). Otherwise, it will be broken.
> > > Is this documented somewhere?  (It wouldn't surprise me if it
> > > is and I just missed it.)
> > > 
> > 
> > Sorry no. It is on my todo list. But I'm getting motivation now.
> > 
> > > This driver has as its origins some vendor code, and I simply
> > > removed the runtime PM calls.  I didn't realize something would
> > > be broken without making pm_runtime*() calls.
> > > 
> > 
> > It is the PM framework requirement to mark the device as 'active' to allow it to
> > participate in runtime PM. If you do not mark it as 'active' and 'enable' it,
> > the framework will not allow propagating the runtime PM changes before *this*
> > device. For instance, consider the generic PCI topology:
> > 
> > PCI controller (platform device)
> > 	|
> > PCI host bridge
> > 	|
> > PCI Root Port
> > 	|
> > PCI endpoint device
> > 
> > If the runtime PM is not enabled for the PCI Root Port, then if the PCI endpoint
> > device runtime suspends, it will not trigger runtime suspend for the Root Port
> > and also for the PCI controller. Also, since the runtime PM framework doesn't
> > have the visibility of the devices underneath the bus (like endpoint), it may
> > assume that no devices (children) are currently active and may trigger runtime
> > suspend of the Root Port (parent) even though the endpoint device could be
> > 'active'.
> 
> So this basically marks this controller as a pass-through device that
> doesn't itself change state for runtime PM, but still communicates that
> somewhere at or below it there might be devices that do participate.

Yes.

- Mani

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

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Manivannan Sadhasivam]

On Thu, Oct 30, 2025 at 06:49:37PM +0100, Aurelien Jarno wrote:
> Hi Mani,
> 
> On 2025-10-30 22:11, Manivannan Sadhasivam wrote:
> > + Aurelien
> > 
> > On Tue, Oct 28, 2025 at 01:48:32PM -0700, Johannes Erdfelt wrote:
> > > On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
> > > > On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
> > > > > I have been testing this patchset recently as well, but on an Orange Pi
> > > > > RV2 board instead (and an extra RV2 specific patch to enable power to
> > > > > the M.2 slot).
> > > > > 
> > > > > I ran into the same symptoms you had ("QID 0 timeout" after about 60
> > > > > seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
> > > > > seems to work fine with the "pcie_aspm=off" workaround as well.
> > > > 
> > > > I don't see this problem, and haven't tried to reproduce it yet.
> > > > 
> > > > Mani told me I needed to add these lines to ensure the "runtime
> > > > PM hierarchy of PCIe chain" won't be "broken":
> > > > 
> > > > 	pm_runtime_set_active()
> > > > 	pm_runtime_no_callbacks()
> > > > 	devm_pm_runtime_enable()
> > > > 
> > > > Just out of curiosity, could you try with those lines added
> > > > just before these assignments in k1_pcie_probe()?
> > > > 
> > > > 	k1->pci.dev = dev;
> > > > 	k1->pci.ops = &k1_pcie_ops;
> > > > 	dw_pcie_cap_set(&k1->pci, REQ_RES);
> > > > 
> > > > I doubt it will fix what you're seeing, but at the moment I'm
> > > > working on something else.
> > > 
> > > Unfortunately there is no difference with the runtime PM hierarchy
> > > additions.
> > > 
> > 
> > These are not supposed to fix the issues you were facing. I discussed with Alex
> > offline and figured out that L1 works fine on his BPI-F3 board with a NVMe SSD.
> > 
> > And I believe, Aurelien is also using that same board, but with different
> > SSDs. But what is puzzling me is, L1 is breaking Aurelien's setup with 3 SSDs
> > from different vendors. It apparently works fine on Alex's setup. So it somehow
> > confirms that Root Port supports and behaves correctly with L1. But at the same
> > time, I cannot just say without evidence that L1 is broken on all these SSDs
> > that you and Aurelien tested with.
> 
> It could be that we have different revision of the BPI-F3 board, it's 
> not impossible that I got an early-ish version. That said I just 
> visually checked the PCB against the schematics, and the devices on the 
> CLKREQN line appear to be installed.
> 

CLKREQ# is only needed for L1 PM Substates (L1.1 and L1.2). In other ASPM states
(L0s and L1), REFCLK is supposed to be ON. So those don't need CLKREQ# assertion
by the endpoint.

The L1 issue you are facing could be due to the board routing issue also. I'm
just speculating here.

> If someone has contacts to check what changes have been done between the 
> different board revision, that could help. Or same if there are 
> different revisions of the SpacemiT K1 chip.
> 

I hope Alex can get this information.

- Mani

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

Re: [PATCH v2 5/7] PCI: spacemit: introduce SpacemiT PCIe host driver

[Alex Elder]

On 10/31/25 1:05 AM, Manivannan Sadhasivam wrote:
> On Wed, Oct 29, 2025 at 07:10:10PM -0500, Alex Elder wrote:
>> On 10/28/25 2:06 AM, Manivannan Sadhasivam wrote:
>>> On Mon, Oct 27, 2025 at 05:24:38PM -0500, Alex Elder wrote:
>>>> On 10/26/25 11:55 AM, Manivannan Sadhasivam wrote:
>>>>> On Mon, Oct 13, 2025 at 10:35:22AM -0500, Alex Elder wrote:
>>>>>> Introduce a driver for the PCIe host controller found in the SpacemiT
>>>>>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>>>>>> The driver supports three PCIe ports that operate at PCIe gen2 transfer
>>>>>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>>>>>> configured for use for USB 3 instead.
>>>>>>
>>>>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>>>>> ---
>>
>> . . .
>>
>>>>>> +	ret = devm_regulator_get_enable(dev, "vpcie3v3-supply");
>>>>>> +	if (ret)
>>>>>> +		return dev_err_probe(dev, ret,
>>>>>> +				     "failed to get \"vpcie3v3\" supply\n");
>>>>>
>>>>> As mentioned in the bindings patch, you should rely on the PWRCTRL_SLOT driver
>>>>> to handle the power supplies. It is not yet handling the PERST#, but I have a
>>>>> series floating for that:
>>>>> https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/
>>>>
>>>> I think that just means that I'll define a DT node compatible with
>>>> "pciclass,0604", and in that node I'll specify the vpcie3v3-supply
>>>> property.  That will cause that (pwrctrl) device to get and enable
>>>> the supply before the "real" PCIe device probes.
>>>>
>>>
>>> Right.
>>>
>>>> And once your PERST work gets merged into the PCI power control
>>>> framework, a callback will allow that to assert PERST# as needed
>>>> surrounding power transitions.  (But I won't worry about that
>>>> for now.)
>>>>
>>>
>>> I'm still nervous to say that you should not worry about it (about not
>>> deasserting PERST# at the right time) as it goes against the PCIe spec.
>>> Current pwrctrl platforms supporting PERST# are working fine due to sheer luck.
>>>
>>> So it would be better to leave the pwrctrl driver out of the equation now and
>>> enable the supply in this driver itself. Later, once my pwrctrl rework gets
>>> merged, I will try to switch this driver to use it.
>>
>> As I understand it, PERST# should be only be deasserted after
>> all power rails are known to be stable.
>>
> 
> Yes
> 
>> This driver enables the regulator during probe, shortly
>> before calling dw_pcie_host_init().  That function calls
>> back to k1_pcie_init(), which enables clocks, deasserts
>> resets, and initializes the PHY before it changes the
>> PERST# state.
>>
>> By "changing the PERST# state" I mean it is asserted
>> (driven low), then deasserted after 100 milliseconds
>> (PCIE_T_PVPERL_MS).
>>
>> I have two questions on this:
>> - You say the PCI spec talks about the "right time" to
>>    deassert PERST# (relative to power).  Is that at all
>>    related to PCIE_T_PVPERL_MS?
> 
> The PCI CEM spec says that PERST# should be deasserted atleast 100ms after the
> power becomes stable. But with the current pwrctrl design, the host controller

So it *is* related to that delay, but the concern you have is about
the pwrctrl design.  Simply probing the pwrctrl device enables all
its regulators, but that probe is not synchronized with the host
controller driver.  In this driver, PERST# is deasserted in the
dw_pcie_host_ops->init callback, which could be called *before*
the pwrctrl probe, and we want it to occur only *after* it.

I find on my system that (at least based on printk() time stamps)
the PERST# is deasserted *before* regulator_bulk_enable() in
pci_pwrctrl_slot_probe() has completed.  So you're absolutely
right about the problem.  The NVMe device doesn't probe until
at least 5 seconds later.

I don't want to rely on this sketchy behavior, so here is what
I plan to do (please tell me whether you agree).

I will back out the change that moves the regulator into the
root port.  I will still use the pwrctrl model--meaning the
root port will still be compatible with "pciclass,0604"--but
because it will define no regulators or clocks, it just won't
do anything of value for now.

Later, when you add the callback to the pwrctrl framework,
we can implement that callback and then move the regulator
definitions into the root port, and no longer enable them
in the host controller driver.

> deasserts the PERST# even before the pwrctrl probe. So this is in violation of
> the spec. But depending on the endpoint device design, this might not cause any
> issue as PERST# is a level triggered signal. So once the endpoint boots up, it
> will see the PERST# deassert and will start working. I'm not justifying the
> current design, but just mentioning that you might not see any issue.

I'm not seeing any issue, but it's true, the sequence of events
is not compliant with what you describe from the spec.

> That being said, we are going to submit a series that reworks pwrctrl framework
> such that each controller can call an API to probe pwrctrl drivers. This way,
> host controller driver can make sure that the device will get powered ON before
> it deasserts the PERST#.

For DesignWare-based devices that might mean a new dw_pcie_host_ops
callback function, or maybe it can just defer calling its ->init
callback until it knows power is enabled and stable.

>> - I notice that PERST# is in a deasserted state at the
>>    time I assert it in this sequence.  Do you see any
>>    reason I should assert it early as an initialization
>>    step, or is asserting it and holding it there for
>>    100 msec sufficient?
>>
> 
> You should assert PERST# before doing any controller initialization sequence as
> that may affect the endpoint. Once PERST# is asserted, it will cause the
> endpoint to 'reset'. So you do your initialization sequence and deassert it once
> done.

Basically the probe function does basic setup, then calls
dw_pcie_host_init().  The next thing that's expected is
the ->init callback from the DesignWare core code, and
that's where PERST# is asserted, then deasserted after
some other actions.

I think I'll assert PERST# (and delay 100 msec) a little
earlier though.

. . .

>>>> In dw_handle_msi_irq(), "num_ctrls" is computed based on
>>>> num_vectors / MAX_MSI_IRQS_PER_CTRL=32.  A loop then
>>>> iterates over those "controllers"(?) to handle each bit
>>>> set in their corresponding register.
>>>>
>>>> This seems OK.  Can you explain why you think it's wrong?
>>>>
>>>
>>> So both 'ctrl' and 'msi' IRQs are interrelated. Each 'ctrl' can have upto 32 MSI
>>> vectors only. If your platform supports more than 32 MSI vectors, like 256, then
>>> the platform DT should provide 8 'msi' IRQs.
>>
>> I have asked SpacemiT about this, specifically whether there
>> are additional interrupts (I don't think there are), or if
>> not that, additional registers to support MSI 32+ (see
>> below).  In their downstream driver they handle interrupts
>> differently.  I suspect num_vectors needs to be set to 32
>> (or I'll leave it unset and take the default).

This was changed to use the default (32 MSI vectors) in v4
of this series.  I haven't heard back from SpacemiT but I'm
pretty sure this is correct.

>> In the DesignWare driver, there are up to 8 "ctrls", and each
>> ctrl has 32 bit positions representing 32 MSI vectors.  Each
>> can have an msi_irq defined.  An msi_irq is always set up for
>> ctrl 0.
>>
>> For any ctrl with an msi_irq assigned, dw_pcie_msi_host_init()
>> sets its interrupt handler to dw_chained_msi_isr(), which just
>> calls dw_handle_msi_irq().
>>
>> The way dw_handle_msi_irq() works, a single ctrl apparently can
>> handle up to 256 MSI vectors, as long as the block of 3 registers
>> that manage the ctrl (ENABLE, MASK, and STATUS presumably) are
>> consecutive in I/O memory for consecutive ctrls.
>>
> 
> I'm not sure how you came up with this observation. dw_handle_msi_irq() loops
> over the 'status' using find_next_bit() of size MAX_MSI_IRQS_PER_CTRL, which is
> 32. So I don't see how a single ctrl can handle up to 256 vectors.

This doesn't matter, because you say it's not correct, but I'll
explain what I meant.

I'm saying *if* there were 8 consecutive sets of 3 registers for
these ctrls:

           ----------
ctrl0:    | ENABLE |
           |--------|
           | STATUS |
           |--------|
           |  MASK  |
           |--------|
ctrl1:    | ENABLE |
           |--------|
           | STATUS |
           |--------|
           |  MASK  |
           |--------|
              ...

then they could all be handled by a single interrupt, based on
the way the loop works.

If every ctrl has its own interrupt, then the interrupt handler
could just handle the one ctrl's 32 possible interrupts.

This loop structure is why I thought it was OK to have 256 MSI
vectors with the one handler.

>> I looked for other examples.  I see that "pcie-fu740.c", which
>> supports compatible "sifive,fu740-pcie", sets num_vectors to
>> MAX_MSI_IRQS, but "fu740-c000.dtsi" defines just one "msi"
>> interrupt.  And "pci-dra7xx.c" seems to do something similar,
>> and maybe "pcie-rcar-gen4.c" too.
>>
> 
> Yes. But I think those are mistakes. I will submit patches to fix them.
OK.  They reinforced my thought that this was doing the right
thing.  The warning you added makes it clear it is not.

Thank you very much for your explanation Mani.

					-Alex

>>> Currently the driver is not strict about this requirement. I will send a patch
>>> to print an error message if this requirement is not satisfied.
>>>
>>>>>> +
>>>>>> +	platform_set_drvdata(pdev, k1);
>>>>>> +
>>>>>
>>>>> For setting the correct runtime PM state of the controller, you should do:
>>>>>
>>>>> pm_runtime_set_active()
>>>>> pm_runtime_no_callbacks()
>>>>> devm_pm_runtime_enable()
>>>>
>>>> OK, that's easy enough.
>>>>
>>>>> This will fix the runtime PM hierarchy of PCIe chain (from host controller to
>>>>> client drivers). Otherwise, it will be broken.
>>>> Is this documented somewhere?  (It wouldn't surprise me if it
>>>> is and I just missed it.)
>>>>
>>>
>>> Sorry no. It is on my todo list. But I'm getting motivation now.
>>>
>>>> This driver has as its origins some vendor code, and I simply
>>>> removed the runtime PM calls.  I didn't realize something would
>>>> be broken without making pm_runtime*() calls.
>>>>
>>>
>>> It is the PM framework requirement to mark the device as 'active' to allow it to
>>> participate in runtime PM. If you do not mark it as 'active' and 'enable' it,
>>> the framework will not allow propagating the runtime PM changes before *this*
>>> device. For instance, consider the generic PCI topology:
>>>
>>> PCI controller (platform device)
>>> 	|
>>> PCI host bridge
>>> 	|
>>> PCI Root Port
>>> 	|
>>> PCI endpoint device
>>>
>>> If the runtime PM is not enabled for the PCI Root Port, then if the PCI endpoint
>>> device runtime suspends, it will not trigger runtime suspend for the Root Port
>>> and also for the PCI controller. Also, since the runtime PM framework doesn't
>>> have the visibility of the devices underneath the bus (like endpoint), it may
>>> assume that no devices (children) are currently active and may trigger runtime
>>> suspend of the Root Port (parent) even though the endpoint device could be
>>> 'active'.
>>
>> So this basically marks this controller as a pass-through device that
>> doesn't itself change state for runtime PM, but still communicates that
>> somewhere at or below it there might be devices that do participate.
> 
> Yes.
> 
> - Mani

Re: [PATCH v2 0/7] Introduce SpacemiT K1 PCIe phy and host controller

[Alex Elder]

On 10/31/25 1:10 AM, Manivannan Sadhasivam wrote:
> On Thu, Oct 30, 2025 at 06:49:37PM +0100, Aurelien Jarno wrote:
>> Hi Mani,
>>
>> On 2025-10-30 22:11, Manivannan Sadhasivam wrote:
>>> + Aurelien
>>>
>>> On Tue, Oct 28, 2025 at 01:48:32PM -0700, Johannes Erdfelt wrote:
>>>> On Tue, Oct 28, 2025, Alex Elder <elder@riscstar.com> wrote:
>>>>> On 10/28/25 1:42 PM, Johannes Erdfelt wrote:
>>>>>> I have been testing this patchset recently as well, but on an Orange Pi
>>>>>> RV2 board instead (and an extra RV2 specific patch to enable power to
>>>>>> the M.2 slot).
>>>>>>
>>>>>> I ran into the same symptoms you had ("QID 0 timeout" after about 60
>>>>>> seconds). However, I'm using an Intel 600p. I can confirm my NVME drive
>>>>>> seems to work fine with the "pcie_aspm=off" workaround as well.
>>>>>
>>>>> I don't see this problem, and haven't tried to reproduce it yet.
>>>>>
>>>>> Mani told me I needed to add these lines to ensure the "runtime
>>>>> PM hierarchy of PCIe chain" won't be "broken":
>>>>>
>>>>> 	pm_runtime_set_active()
>>>>> 	pm_runtime_no_callbacks()
>>>>> 	devm_pm_runtime_enable()
>>>>>
>>>>> Just out of curiosity, could you try with those lines added
>>>>> just before these assignments in k1_pcie_probe()?
>>>>>
>>>>> 	k1->pci.dev = dev;
>>>>> 	k1->pci.ops = &k1_pcie_ops;
>>>>> 	dw_pcie_cap_set(&k1->pci, REQ_RES);
>>>>>
>>>>> I doubt it will fix what you're seeing, but at the moment I'm
>>>>> working on something else.
>>>>
>>>> Unfortunately there is no difference with the runtime PM hierarchy
>>>> additions.
>>>>
>>>
>>> These are not supposed to fix the issues you were facing. I discussed with Alex
>>> offline and figured out that L1 works fine on his BPI-F3 board with a NVMe SSD.
>>>
>>> And I believe, Aurelien is also using that same board, but with different
>>> SSDs. But what is puzzling me is, L1 is breaking Aurelien's setup with 3 SSDs
>>> from different vendors. It apparently works fine on Alex's setup. So it somehow
>>> confirms that Root Port supports and behaves correctly with L1. But at the same
>>> time, I cannot just say without evidence that L1 is broken on all these SSDs
>>> that you and Aurelien tested with.

Aurelien, can you please confirm that your reports are with the BPI-F3
board?  I believe you identified the three SSDs that were failing.  I
am considering buying one of those models to see if I can reproduce
the problem and troubleshoot it.

>> It could be that we have different revision of the BPI-F3 board, it's
>> not impossible that I got an early-ish version. That said I just
>> visually checked the PCB against the schematics, and the devices on the
>> CLKREQN line appear to be installed.
>>
> 
> CLKREQ# is only needed for L1 PM Substates (L1.1 and L1.2). In other ASPM states
> (L0s and L1), REFCLK is supposed to be ON. So those don't need CLKREQ# assertion
> by the endpoint.
> 
> The L1 issue you are facing could be due to the board routing issue also. I'm
> just speculating here.
> 
>> If someone has contacts to check what changes have been done between the
>> different board revision, that could help. Or same if there are
>> different revisions of the SpacemiT K1 chip.
>>
> 
> I hope Alex can get this information.

I have sent a message to SpacemiT to explain that these issues are
being reported, and asking for any useful information about the
BPI-F3 (including whether there are different versions, or different
versions of firmware, and how someone can identify what they have).

Thanks.

					-Alex

> - Mani
>