[PATCH 0/2] Add firmware-managed PCIe Endpoint support for SA8255P

[PATCH 0/2] Add firmware-managed PCIe Endpoint support for SA8255P

[Mrinmay Sarkar]

This patch series introduces support for Qualcomm SA8255P platform
where PCIe Endpoint resources are managed by firmware instead of
Linux driver. So the Linux driver should avoid redundant resource
management and relies on runtime PM calls to inform firmware for
resource management.

And documents the new compatible string "qcom,sa8255p-pcie-ep" for
SA8255P platforms in the device tree bindings.

Tested on Qualcomm SA8255P platform.

Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
---
Mrinmay Sarkar (2):
      dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint
      PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

 .../bindings/pci/qcom,pcie-ep-sa8255p.yaml         | 114 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c          |  80 ++++++++++++---
 2 files changed, 178 insertions(+), 16 deletions(-)
---
base-commit: b2c27842ba853508b0da00187a7508eb3a96c8f7
change-id: 20251203-firmware_managed_ep-37f1efb69b15

Best regards,
-- 
Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>

[PATCH 1/2] dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint

[Mrinmay Sarkar]

Document the required configuration to enable the PCIe Endpoint controller
on SA8255p which is managed by firmware using power-domain based handling.

Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
---
 .../bindings/pci/qcom,pcie-ep-sa8255p.yaml         | 114 +++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..970f65d46c8e2fa4c44665cb7a346dea1dc9e06a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/qcom,pcie-ep-sa8255p.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm firmware managed PCIe Endpoint Controller
+
+description:
+  Qualcomm SA8255p SoC PCIe endpoint controller is based on the Synopsys
+  DesignWare PCIe IP which is managed by firmware.
+
+maintainers:
+  - Manivannan Sadhasivam <mani@kernel.org>
+
+properties:
+  compatible:
+    const: qcom,sa8255p-pcie-ep
+
+  reg:
+    minItems: 6
+    items:
+      - description: Qualcomm-specific PARF configuration registers
+      - description: DesignWare PCIe registers
+      - description: External local bus interface registers
+      - description: Address Translation Unit (ATU) registers
+      - description: Memory region used to map remote RC address space
+      - description: BAR memory region
+      - description: DMA register space
+
+  reg-names:
+    minItems: 6
+    items:
+      - const: parf
+      - const: dbi
+      - const: elbi
+      - const: atu
+      - const: addr_space
+      - const: mmio
+      - const: dma
+
+  interrupts:
+    minItems: 2
+    items:
+      - description: PCIe Global interrupt
+      - description: PCIe Doorbell interrupt
+      - description: DMA interrupt
+
+  interrupt-names:
+    minItems: 2
+    items:
+      - const: global
+      - const: doorbell
+      - const: dma
+
+  iommus:
+    maxItems: 1
+
+  reset-gpios:
+    description: GPIO used as PERST# input signal
+    maxItems: 1
+
+  wake-gpios:
+    description: GPIO used as WAKE# output signal
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  dma-coherent: true
+
+  num-lanes:
+    default: 2
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - reset-gpios
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        pcie1_ep: pcie-ep@1c10000 {
+            compatible = "qcom,sa8255p-pcie-ep";
+            reg = <0x0 0x01c10000 0x0 0x3000>,
+                  <0x0 0x60000000 0x0 0xf20>,
+                  <0x0 0x60000f20 0x0 0xa8>,
+                  <0x0 0x60001000 0x0 0x4000>,
+                  <0x0 0x60200000 0x0 0x100000>,
+                  <0x0 0x01c13000 0x0 0x1000>,
+                  <0x0 0x60005000 0x0 0x2000>;
+            reg-names = "parf", "dbi", "elbi", "atu", "addr_space", "mmio", "dma";
+            interrupts = <GIC_SPI 518 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "global", "doorbell", "dma";
+            reset-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
+            wake-gpios = <&tlmm 5 GPIO_ACTIVE_LOW>;
+            dma-coherent;
+            iommus = <&pcie_smmu 0x80 0x7f>;
+            power-domains = <&scmi6_pd 1>;
+            num-lanes = <4>;
+        };
+    };

-- 
2.25.1

[PATCH 2/2] PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

[Mrinmay Sarkar]

Some Qualcomm platforms use firmware to manage PCIe resources such as
clocks, resets, and PHY through the SCMI interface. In these cases,
the Linux driver should not perform resource enable or disable
operations directly. Additionally, runtime PM support has been enabled
to ensure proper power state transitions.

This commit introduces a `firmware_managed` flag in the Endpoint
configuration structure. When set, the driver skips resource handling
and uses generic runtime PM calls to let firmware do resource management.

A new compatible string is added for SA8255P platforms where firmware
manages resources.

Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 80 ++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index f1bc0ac81a928b928ab3f8cc7bf82558fc430474..38358c9fa7ab32fd36efcea0a42c52f1f86a523a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -168,11 +168,13 @@ enum qcom_pcie_ep_link_status {
  * @hdma_support: HDMA support on this SoC
  * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache snooping
  * @disable_mhi_ram_parity_check: Disable MHI RAM data parity error check
+ * @firmware_managed: Set if the Endpoint controller is firmware managed
  */
 struct qcom_pcie_ep_cfg {
 	bool hdma_support;
 	bool override_no_snoop;
 	bool disable_mhi_ram_parity_check;
+	bool firmware_managed;
 };
 
 /**
@@ -377,6 +379,15 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
 
 static void qcom_pcie_disable_resources(struct qcom_pcie_ep *pcie_ep)
 {
+	struct device *dev = pcie_ep->pci.dev;
+	int ret;
+
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
+		return;
+	}
+
 	icc_set_bw(pcie_ep->icc_mem, 0, 0);
 	phy_power_off(pcie_ep->phy);
 	phy_exit(pcie_ep->phy);
@@ -390,12 +401,22 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 	u32 val, offset;
 	int ret;
 
-	ret = qcom_pcie_enable_resources(pcie_ep);
-	if (ret) {
-		dev_err(dev, "Failed to enable resources: %d\n", ret);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
 		return ret;
 	}
 
+	/* Enable resources if Endpoint controller is not firmware-managed */
+	if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
+		ret = qcom_pcie_enable_resources(pcie_ep);
+		if (ret) {
+			dev_err(dev, "Failed to enable resources: %d\n", ret);
+			pm_runtime_put_sync(dev);
+			return ret;
+		}
+	}
+
 	/* Perform cleanup that requires refclk */
 	pci_epc_deinit_notify(pci->ep.epc);
 	dw_pcie_ep_cleanup(&pci->ep);
@@ -630,16 +651,6 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
 		return ret;
 	}
 
-	pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
-	if (pcie_ep->num_clks < 0) {
-		dev_err(dev, "Failed to get clocks\n");
-		return pcie_ep->num_clks;
-	}
-
-	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
-	if (IS_ERR(pcie_ep->core_reset))
-		return PTR_ERR(pcie_ep->core_reset);
-
 	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
 	if (IS_ERR(pcie_ep->reset))
 		return PTR_ERR(pcie_ep->reset);
@@ -652,9 +663,22 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
 	if (IS_ERR(pcie_ep->phy))
 		ret = PTR_ERR(pcie_ep->phy);
 
-	pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
-	if (IS_ERR(pcie_ep->icc_mem))
-		ret = PTR_ERR(pcie_ep->icc_mem);
+	/* Populate resources if Endpoint controller is not firmware-managed */
+	if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
+		pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
+		if (pcie_ep->num_clks < 0) {
+			dev_err(dev, "Failed to get clocks\n");
+			return pcie_ep->num_clks;
+		}
+
+		pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
+		if (IS_ERR(pcie_ep->core_reset))
+			return PTR_ERR(pcie_ep->core_reset);
+
+		pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
+		if (IS_ERR(pcie_ep->icc_mem))
+			ret = PTR_ERR(pcie_ep->icc_mem);
+	}
 
 	return ret;
 }
@@ -874,6 +898,16 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie_ep);
 
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
+		return ret;
+	}
+
 	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
 	if (ret)
 		return ret;
@@ -897,6 +931,12 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
 	pcie_ep->debugfs = debugfs_create_dir(name, NULL);
 	qcom_pcie_ep_init_debugfs(pcie_ep);
 
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
+		goto err_disable_irqs;
+	}
+
 	return 0;
 
 err_disable_irqs:
@@ -930,7 +970,15 @@ static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
 	.disable_mhi_ram_parity_check = true,
 };
 
+static const struct qcom_pcie_ep_cfg cfg_1_34_0_fw_managed = {
+	.hdma_support = true,
+	.override_no_snoop = true,
+	.disable_mhi_ram_parity_check = true,
+	.firmware_managed = true,
+};
+
 static const struct of_device_id qcom_pcie_ep_match[] = {
+	{ .compatible = "qcom,sa8255p-pcie-ep", .data = &cfg_1_34_0_fw_managed},
 	{ .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
 	{ .compatible = "qcom,sdx55-pcie-ep", },
 	{ .compatible = "qcom,sm8450-pcie-ep", },

-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint

[Krzysztof Kozlowski]

On Wed, Dec 03, 2025 at 06:56:47PM +0530, Mrinmay Sarkar wrote:
> Document the required configuration to enable the PCIe Endpoint controller
> on SA8255p which is managed by firmware using power-domain based handling.
> 
> Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
> ---
>  .../bindings/pci/qcom,pcie-ep-sa8255p.yaml         | 114 +++++++++++++++++++++

Filename must match the compatible. In your case, the filename is
correct but you wanted old format for the compatible (so compatible
should be rewritten to match filename).

>  1 file changed, 114 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..970f65d46c8e2fa4c44665cb7a346dea1dc9e06a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep-sa8255p.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm firmware managed PCIe Endpoint Controller
> +
> +description:
> +  Qualcomm SA8255p SoC PCIe endpoint controller is based on the Synopsys
> +  DesignWare PCIe IP which is managed by firmware.
> +
> +maintainers:
> +  - Manivannan Sadhasivam <mani@kernel.org>
> +
> +properties:
> +  compatible:
> +    const: qcom,sa8255p-pcie-ep
> +
> +  reg:
> +    minItems: 6

Why is this flexible?

> +    items:
> +      - description: Qualcomm-specific PARF configuration registers
> +      - description: DesignWare PCIe registers
> +      - description: External local bus interface registers
> +      - description: Address Translation Unit (ATU) registers
> +      - description: Memory region used to map remote RC address space
> +      - description: BAR memory region
> +      - description: DMA register space
> +
> +  reg-names:
> +    minItems: 6
> +    items:
> +      - const: parf
> +      - const: dbi
> +      - const: elbi
> +      - const: atu
> +      - const: addr_space
> +      - const: mmio
> +      - const: dma
> +
> +  interrupts:
> +    minItems: 2

And this/

> +    items:
> +      - description: PCIe Global interrupt
> +      - description: PCIe Doorbell interrupt
> +      - description: DMA interrupt
> +
> +  interrupt-names:
> +    minItems: 2
> +    items:
> +      - const: global
> +      - const: doorbell
> +      - const: dma
> +
> +  iommus:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: GPIO used as PERST# input signal
> +    maxItems: 1
> +
> +  wake-gpios:
> +    description: GPIO used as WAKE# output signal
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  dma-coherent: true
> +
> +  num-lanes:
> +    default: 2

Isn't this deducible from the compatible? Do you have have different
PCIe controllers with different lanes?


> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - reset-gpios
> +  - power-domains
> +
> +additionalProperties: false

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint

[Mrinmay Sarkar]

On Fri, Dec 5, 2025 at 2:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, Dec 03, 2025 at 06:56:47PM +0530, Mrinmay Sarkar wrote:
> > Document the required configuration to enable the PCIe Endpoint controller
> > on SA8255p which is managed by firmware using power-domain based handling.
> >
> > Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
> > ---
> >  .../bindings/pci/qcom,pcie-ep-sa8255p.yaml         | 114 +++++++++++++++++++++
>
> Filename must match the compatible. In your case, the filename is
> correct but you wanted old format for the compatible (so compatible
> should be rewritten to match filename).

Thanks Krzysztof for the review.
I will fix the compatible string to match the filename (`qcom,pcie-ep-sa8255p`).

>
> >  1 file changed, 114 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..970f65d46c8e2fa4c44665cb7a346dea1dc9e06a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep-sa8255p.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm firmware managed PCIe Endpoint Controller
> > +
> > +description:
> > +  Qualcomm SA8255p SoC PCIe endpoint controller is based on the Synopsys
> > +  DesignWare PCIe IP which is managed by firmware.
> > +
> > +maintainers:
> > +  - Manivannan Sadhasivam <mani@kernel.org>
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,sa8255p-pcie-ep
> > +
> > +  reg:
> > +    minItems: 6
>
> Why is this flexible?

The reason for `minItems: 6` is that the DMA register space can be
skipped if DMA is not used.

>
> > +    items:
> > +      - description: Qualcomm-specific PARF configuration registers
> > +      - description: DesignWare PCIe registers
> > +      - description: External local bus interface registers
> > +      - description: Address Translation Unit (ATU) registers
> > +      - description: Memory region used to map remote RC address space
> > +      - description: BAR memory region
> > +      - description: DMA register space
> > +
> > +  reg-names:
> > +    minItems: 6
> > +    items:
> > +      - const: parf
> > +      - const: dbi
> > +      - const: elbi
> > +      - const: atu
> > +      - const: addr_space
> > +      - const: mmio
> > +      - const: dma
> > +
> > +  interrupts:
> > +    minItems: 2
>
> And this/

Similarly, DMA interrupt can be skipped if DMA is not used.

>
> > +    items:
> > +      - description: PCIe Global interrupt
> > +      - description: PCIe Doorbell interrupt
> > +      - description: DMA interrupt
> > +
> > +  interrupt-names:
> > +    minItems: 2
> > +    items:
> > +      - const: global
> > +      - const: doorbell
> > +      - const: dma
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: GPIO used as PERST# input signal
> > +    maxItems: 1
> > +
> > +  wake-gpios:
> > +    description: GPIO used as WAKE# output signal
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  dma-coherent: true
> > +
> > +  num-lanes:
> > +    default: 2
>
> Isn't this deducible from the compatible? Do you have have different
> PCIe controllers with different lanes?

SA8255p has 2 pcie controllers(pcie0 and pcie1).
pcie0 supports 2 lanes, and pcie1 supports 4 lanes.

-Mrinmay
>
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - reset-gpios
> > +  - power-domains
> > +
> > +additionalProperties: false
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint

[Krzysztof Kozlowski]

On 05/12/2025 13:58, Mrinmay Sarkar wrote:
>>>  1 file changed, 114 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..970f65d46c8e2fa4c44665cb7a346dea1dc9e06a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
>>> @@ -0,0 +1,114 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep-sa8255p.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm firmware managed PCIe Endpoint Controller
>>> +
>>> +description:
>>> +  Qualcomm SA8255p SoC PCIe endpoint controller is based on the Synopsys
>>> +  DesignWare PCIe IP which is managed by firmware.
>>> +
>>> +maintainers:
>>> +  - Manivannan Sadhasivam <mani@kernel.org>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,sa8255p-pcie-ep
>>> +
>>> +  reg:
>>> +    minItems: 6
>>
>> Why is this flexible?
> 
> The reason for `minItems: 6` is that the DMA register space can be
> skipped if DMA is not used.

But the hardware has this anyway, so this must be here. You do not write
bindings depending how drivers use them in your use case.

Either drop minItems (fixed size of array) or provide rationale in terms
of hardware in commit msg.

...


>>> +
>>> +  dma-coherent: true
>>> +
>>> +  num-lanes:
>>> +    default: 2
>>
>> Isn't this deducible from the compatible? Do you have have different
>> PCIe controllers with different lanes?
> 
> SA8255p has 2 pcie controllers(pcie0 and pcie1).
> pcie0 supports 2 lanes, and pcie1 supports 4 lanes.

That's ok, thanks.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: PCI: qcom,pcie-ep-sa8255p: Document firmware managed PCIe endpoint

[Mrinmay Sarkar]

On Fri, Dec 5, 2025 at 6:44 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/12/2025 13:58, Mrinmay Sarkar wrote:
> >>>  1 file changed, 114 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..970f65d46c8e2fa4c44665cb7a346dea1dc9e06a
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep-sa8255p.yaml
> >>> @@ -0,0 +1,114 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep-sa8255p.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Qualcomm firmware managed PCIe Endpoint Controller
> >>> +
> >>> +description:
> >>> +  Qualcomm SA8255p SoC PCIe endpoint controller is based on the Synopsys
> >>> +  DesignWare PCIe IP which is managed by firmware.
> >>> +
> >>> +maintainers:
> >>> +  - Manivannan Sadhasivam <mani@kernel.org>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: qcom,sa8255p-pcie-ep
> >>> +
> >>> +  reg:
> >>> +    minItems: 6
> >>
> >> Why is this flexible?
> >
> > The reason for `minItems: 6` is that the DMA register space can be
> > skipped if DMA is not used.
>
> But the hardware has this anyway, so this must be here. You do not write
> bindings depending how drivers use them in your use case.
>
> Either drop minItems (fixed size of array) or provide rationale in terms
> of hardware in commit msg.
>
> ...

Thanks Krzysztof for the feedback.

 As per your suggestion, I will drop the `minItems` property for
`reg`, `reg-names`, `interrupts`, and `interrupt-names` since the
hardware always exposes all regions and interrupt lines.

I will use a fixed-size array with `items` only.

-Mrinmay
>
>
> >>> +
> >>> +  dma-coherent: true
> >>> +
> >>> +  num-lanes:
> >>> +    default: 2
> >>
> >> Isn't this deducible from the compatible? Do you have have different
> >> PCIe controllers with different lanes?
> >
> > SA8255p has 2 pcie controllers(pcie0 and pcie1).
> > pcie0 supports 2 lanes, and pcie1 supports 4 lanes.
>
> That's ok, thanks.
>
>
> Best regards,
> Krzysztof

Re: [PATCH 2/2] PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

[Jeff Johnson]

On 12/3/2025 5:26 AM, Mrinmay Sarkar wrote:
> Some Qualcomm platforms use firmware to manage PCIe resources such as
> clocks, resets, and PHY through the SCMI interface. In these cases,
> the Linux driver should not perform resource enable or disable
> operations directly. Additionally, runtime PM support has been enabled
> to ensure proper power state transitions.
> 
> This commit introduces a `firmware_managed` flag in the Endpoint

please review:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

"Describe your changes in imperative mood"

Re: [PATCH 2/2] PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

[Bjorn Andersson]

On Wed, Dec 03, 2025 at 06:56:48PM +0530, Mrinmay Sarkar wrote:
> Some Qualcomm platforms use firmware to manage PCIe resources such as
> clocks, resets, and PHY through the SCMI interface. In these cases,
> the Linux driver should not perform resource enable or disable
> operations directly. Additionally, runtime PM support has been enabled
> to ensure proper power state transitions.
> 
> This commit introduces a `firmware_managed` flag in the Endpoint
> configuration structure. When set, the driver skips resource handling
> and uses generic runtime PM calls to let firmware do resource management.
> 
> A new compatible string is added for SA8255P platforms where firmware
> manages resources.
> 
> Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 80 ++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index f1bc0ac81a928b928ab3f8cc7bf82558fc430474..38358c9fa7ab32fd36efcea0a42c52f1f86a523a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -168,11 +168,13 @@ enum qcom_pcie_ep_link_status {
>   * @hdma_support: HDMA support on this SoC
>   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache snooping
>   * @disable_mhi_ram_parity_check: Disable MHI RAM data parity error check
> + * @firmware_managed: Set if the Endpoint controller is firmware managed
>   */
>  struct qcom_pcie_ep_cfg {
>  	bool hdma_support;
>  	bool override_no_snoop;
>  	bool disable_mhi_ram_parity_check;
> +	bool firmware_managed;
>  };
>  
>  /**
> @@ -377,6 +379,15 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>  
>  static void qcom_pcie_disable_resources(struct qcom_pcie_ep *pcie_ep)
>  {
> +	struct device *dev = pcie_ep->pci.dev;
> +	int ret;
> +
> +	ret = pm_runtime_put_sync(dev);

What's the benefit of waiting for the put to finish? (i.e. why _sync)

> +	if (ret < 0) {
> +		dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> +		return;

For some reason the pm_runtime_put_sync() failed, so the device's state
is going to remain active. But you prevented the resources below from
being disabled - without returning an error, so nobody knows.

So now the phy refcount etc will be wrong.

> +	}
> +
>  	icc_set_bw(pcie_ep->icc_mem, 0, 0);
>  	phy_power_off(pcie_ep->phy);
>  	phy_exit(pcie_ep->phy);
> @@ -390,12 +401,22 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  	u32 val, offset;
>  	int ret;
>  
> -	ret = qcom_pcie_enable_resources(pcie_ep);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +	ret = pm_runtime_get_sync(dev);

You're missing necessary error handling for pm_runtime_get_sync(), use
pm_runtime_resume_and_get() instead.

> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
>  		return ret;
>  	}
>  
> +	/* Enable resources if Endpoint controller is not firmware-managed */
> +	if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> +		ret = qcom_pcie_enable_resources(pcie_ep);

Now that you're moving the driver to adequately get and put the RPM
state, can't you move the explicit resource management to pm_ops as
well?

> +		if (ret) {
> +			dev_err(dev, "Failed to enable resources: %d\n", ret);
> +			pm_runtime_put_sync(dev);
> +			return ret;
> +		}
> +	}
> +
>  	/* Perform cleanup that requires refclk */
>  	pci_epc_deinit_notify(pci->ep.epc);
>  	dw_pcie_ep_cleanup(&pci->ep);
> @@ -630,16 +651,6 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> -	pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> -	if (pcie_ep->num_clks < 0) {
> -		dev_err(dev, "Failed to get clocks\n");
> -		return pcie_ep->num_clks;
> -	}
> -
> -	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> -	if (IS_ERR(pcie_ep->core_reset))
> -		return PTR_ERR(pcie_ep->core_reset);
> -
>  	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
>  	if (IS_ERR(pcie_ep->reset))
>  		return PTR_ERR(pcie_ep->reset);
> @@ -652,9 +663,22 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
>  	if (IS_ERR(pcie_ep->phy))
>  		ret = PTR_ERR(pcie_ep->phy);
>  
> -	pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> -	if (IS_ERR(pcie_ep->icc_mem))
> -		ret = PTR_ERR(pcie_ep->icc_mem);
> +	/* Populate resources if Endpoint controller is not firmware-managed */
> +	if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> +		pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> +		if (pcie_ep->num_clks < 0) {
> +			dev_err(dev, "Failed to get clocks\n");
> +			return pcie_ep->num_clks;
> +		}
> +
> +		pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> +		if (IS_ERR(pcie_ep->core_reset))
> +			return PTR_ERR(pcie_ep->core_reset);
> +
> +		pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> +		if (IS_ERR(pcie_ep->icc_mem))
> +			ret = PTR_ERR(pcie_ep->icc_mem);
> +	}
>  
>  	return ret;
>  }
> @@ -874,6 +898,16 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pcie_ep);
>  
> +	pm_runtime_set_active(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +	ret = pm_runtime_get_sync(dev);

As the device is already active, this will just bump the reference count
and return. I think the correct way to write this is:

pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);


But to handle the non-fw-managed case, you probably want to just remove
the pm_runtime_set_active() and keep the get_sync(), to allow the
resources to be turned on, thus would though have to happen after you
acquire the resources below.

> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
>  	if (ret)
>  		return ret;
> @@ -897,6 +931,12 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  	pcie_ep->debugfs = debugfs_create_dir(name, NULL);
>  	qcom_pcie_ep_init_debugfs(pcie_ep);

This was last, because we don't care about failures. But now that you're
adding a source of errors below, you need to remove these entries again
if below fails (or keep the debugfs creation last).

>  
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0) {

I don't think this is adequately error handled.

Regards,
Bjorn

> +		dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> +		goto err_disable_irqs;
> +	}
> +
>  	return 0;
>  
>  err_disable_irqs:
> @@ -930,7 +970,15 @@ static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
>  	.disable_mhi_ram_parity_check = true,
>  };
>  
> +static const struct qcom_pcie_ep_cfg cfg_1_34_0_fw_managed = {
> +	.hdma_support = true,
> +	.override_no_snoop = true,
> +	.disable_mhi_ram_parity_check = true,
> +	.firmware_managed = true,
> +};
> +
>  static const struct of_device_id qcom_pcie_ep_match[] = {
> +	{ .compatible = "qcom,sa8255p-pcie-ep", .data = &cfg_1_34_0_fw_managed},
>  	{ .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
>  	{ .compatible = "qcom,sdx55-pcie-ep", },
>  	{ .compatible = "qcom,sm8450-pcie-ep", },
> 
> -- 
> 2.25.1
> 
> 

Re: [PATCH 2/2] PCI: qcom-ep: Add support for firmware-managed PCIe Endpoint

[Mrinmay Sarkar]

On Sat, Dec 6, 2025 at 2:27 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Wed, Dec 03, 2025 at 06:56:48PM +0530, Mrinmay Sarkar wrote:
> > Some Qualcomm platforms use firmware to manage PCIe resources such as
> > clocks, resets, and PHY through the SCMI interface. In these cases,
> > the Linux driver should not perform resource enable or disable
> > operations directly. Additionally, runtime PM support has been enabled
> > to ensure proper power state transitions.
> >
> > This commit introduces a `firmware_managed` flag in the Endpoint
> > configuration structure. When set, the driver skips resource handling
> > and uses generic runtime PM calls to let firmware do resource management.
> >
> > A new compatible string is added for SA8255P platforms where firmware
> > manages resources.
> >
> > Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 80 ++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index f1bc0ac81a928b928ab3f8cc7bf82558fc430474..38358c9fa7ab32fd36efcea0a42c52f1f86a523a 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -168,11 +168,13 @@ enum qcom_pcie_ep_link_status {
> >   * @hdma_support: HDMA support on this SoC
> >   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache snooping
> >   * @disable_mhi_ram_parity_check: Disable MHI RAM data parity error check
> > + * @firmware_managed: Set if the Endpoint controller is firmware managed
> >   */
> >  struct qcom_pcie_ep_cfg {
> >       bool hdma_support;
> >       bool override_no_snoop;
> >       bool disable_mhi_ram_parity_check;
> > +     bool firmware_managed;
> >  };
> >
> >  /**
> > @@ -377,6 +379,15 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
> >
> >  static void qcom_pcie_disable_resources(struct qcom_pcie_ep *pcie_ep)
> >  {
> > +     struct device *dev = pcie_ep->pci.dev;
> > +     int ret;
> > +
> > +     ret = pm_runtime_put_sync(dev);
>
> What's the benefit of waiting for the put to finish? (i.e. why _sync)
>
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> > +             return;
>
> For some reason the pm_runtime_put_sync() failed, so the device's state
> is going to remain active. But you prevented the resources below from
> being disabled - without returning an error, so nobody knows.
>
> So now the phy refcount etc will be wrong.
>

Thanks Bjorn for the review.
I think we can use pm_runtime_put() as we should disable resources
even if it fails.

> > +     }
> > +

And I will add a check here as we don't need below for the
firmware_managed case.

> >       icc_set_bw(pcie_ep->icc_mem, 0, 0);
> >       phy_power_off(pcie_ep->phy);
> >       phy_exit(pcie_ep->phy);
> > @@ -390,12 +401,22 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >       u32 val, offset;
> >       int ret;
> >
> > -     ret = qcom_pcie_enable_resources(pcie_ep);
> > -     if (ret) {
> > -             dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +     ret = pm_runtime_get_sync(dev);
>
> You're missing necessary error handling for pm_runtime_get_sync(), use
> pm_runtime_resume_and_get() instead.
>

Yes, we can use pm_runtime_resume_and_get() here as it will handle
errors safely.

> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
> >               return ret;
> >       }
> >
> > +     /* Enable resources if Endpoint controller is not firmware-managed */
> > +     if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> > +             ret = qcom_pcie_enable_resources(pcie_ep);
>
> Now that you're moving the driver to adequately get and put the RPM
> state, can't you move the explicit resource management to pm_ops as
> well?

Actually we are planning to enable runtime pm_ops in a separate series.
We will be taking care of this in that series.

>
> > +             if (ret) {
> > +                     dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +                     pm_runtime_put_sync(dev);
> > +                     return ret;
> > +             }
> > +     }
> > +
> >       /* Perform cleanup that requires refclk */
> >       pci_epc_deinit_notify(pci->ep.epc);
> >       dw_pcie_ep_cleanup(&pci->ep);
> > @@ -630,16 +651,6 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> >               return ret;
> >       }
> >
> > -     pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> > -     if (pcie_ep->num_clks < 0) {
> > -             dev_err(dev, "Failed to get clocks\n");
> > -             return pcie_ep->num_clks;
> > -     }
> > -
> > -     pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > -     if (IS_ERR(pcie_ep->core_reset))
> > -             return PTR_ERR(pcie_ep->core_reset);
> > -
> >       pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> >       if (IS_ERR(pcie_ep->reset))
> >               return PTR_ERR(pcie_ep->reset);
> > @@ -652,9 +663,22 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> >       if (IS_ERR(pcie_ep->phy))
> >               ret = PTR_ERR(pcie_ep->phy);
> >
> > -     pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> > -     if (IS_ERR(pcie_ep->icc_mem))
> > -             ret = PTR_ERR(pcie_ep->icc_mem);
> > +     /* Populate resources if Endpoint controller is not firmware-managed */
> > +     if (!(pcie_ep->cfg && pcie_ep->cfg->firmware_managed)) {
> > +             pcie_ep->num_clks = devm_clk_bulk_get_all(dev, &pcie_ep->clks);
> > +             if (pcie_ep->num_clks < 0) {
> > +                     dev_err(dev, "Failed to get clocks\n");
> > +                     return pcie_ep->num_clks;
> > +             }
> > +
> > +             pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > +             if (IS_ERR(pcie_ep->core_reset))
> > +                     return PTR_ERR(pcie_ep->core_reset);
> > +
> > +             pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
> > +             if (IS_ERR(pcie_ep->icc_mem))
> > +                     ret = PTR_ERR(pcie_ep->icc_mem);
> > +     }
> >
> >       return ret;
> >  }
> > @@ -874,6 +898,16 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >
> >       platform_set_drvdata(pdev, pcie_ep);
> >
> > +     pm_runtime_set_active(dev);
> > +     ret = devm_pm_runtime_enable(dev);
> > +     if (ret)
> > +             return ret;
> > +     ret = pm_runtime_get_sync(dev);
>
> As the device is already active, this will just bump the reference count
> and return. I think the correct way to write this is:
>
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>

Yes, here pm_runtime_get_sync() is just incrementing the usage_count
as the device is already active.
we can use pm_runtime_get_noresume() instead.

The reason I was using devm_pm_runtime_enable() is because it
automatically disables runtime PM
in case of probe failure. Please let  me know your thoughts on this.

>
> But to handle the non-fw-managed case, you probably want to just remove
> the pm_runtime_set_active() and keep the get_sync(), to allow the
> resources to be turned on, thus would though have to happen after you
> acquire the resources below.
>
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable endpoint device: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> >       ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> >       if (ret)
> >               return ret;
> > @@ -897,6 +931,12 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >       pcie_ep->debugfs = debugfs_create_dir(name, NULL);
> >       qcom_pcie_ep_init_debugfs(pcie_ep);
>
> This was last, because we don't care about failures. But now that you're
> adding a source of errors below, you need to remove these entries again
> if below fails (or keep the debugfs creation last).

I will move debugfs creation last.

Thanks,
Mrinmay

>
> >
> > +     ret = pm_runtime_put_sync(dev);
> > +     if (ret < 0) {
>
> I don't think this is adequately error handled.
>
> Regards,
> Bjorn
>
> > +             dev_err(dev, "Failed to disable endpoint device: %d\n", ret);
> > +             goto err_disable_irqs;
> > +     }
> > +
> >       return 0;
> >
> >  err_disable_irqs:
> > @@ -930,7 +970,15 @@ static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
> >       .disable_mhi_ram_parity_check = true,
> >  };
> >
> > +static const struct qcom_pcie_ep_cfg cfg_1_34_0_fw_managed = {
> > +     .hdma_support = true,
> > +     .override_no_snoop = true,
> > +     .disable_mhi_ram_parity_check = true,
> > +     .firmware_managed = true,
> > +};
> > +
> >  static const struct of_device_id qcom_pcie_ep_match[] = {
> > +     { .compatible = "qcom,sa8255p-pcie-ep", .data = &cfg_1_34_0_fw_managed},
> >       { .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
> >       { .compatible = "qcom,sdx55-pcie-ep", },
> >       { .compatible = "qcom,sm8450-pcie-ep", },
> >
> > --
> > 2.25.1
> >
> >