[PATCH v2 0/2] PCI: qcom: Binding fix

[PATCH v2 0/2] PCI: qcom: Binding fix

[Manivannan Sadhasivam]

Hi,

This series fixes the binding issue around the PERST# and PHY properties.
The binding issue was reported in [1], while discussing a DTS fix [2].

The binding fix provided in this series is not sufficient enough to spot all the
shenanigans, especially keeping one property in Controller node and another in
Root Port node. But this series does catch the DTS issue fixed by [2]:

arch/arm64/boot/dts/qcom/sm8750-mtp.dtb: pcie@1c00000 (qcom,pcie-sm8750): 'oneOf' conditional failed, one must be fixed:
	'phys' is a required property
	False schema does not allow [[148, 102, 1]]
	False schema does not allow [[148, 104, 0]]
	from schema $id: http://devicetree.org/schemas/pci/qcom,pcie-sm8550.yaml#
arch/arm64/boot/dts/qcom/sm8750-mtp.dtb: pcie@1c00000 (qcom,pcie-sm8750): pcie@0: 'oneOf' conditional failed, one must be fixed:
	'reset-gpios' is a required property
	'wake-gpios' is a required property
	False schema does not allow [[34]]
	from schema $id: http://devicetree.org/schemas/pci/qcom,pcie-sm8550.yaml#
arch/arm64/boot/dts/qcom/sm8750-mtp.dtb: pcie@1c00000 (qcom,pcie-sm8750): Unevaluated properties are not allowed ('#address-cells', '#interrupt-cells', '#size-cells', 'bus-range', 'device_type', 'dma-coherent', 'interconnect-names', 'interconnects', 'interrupt-map', 'interrupt-map-mask', 'iommu-map', 'linux,pci-domain', 'msi-map', 'msi-map-mask', 'num-lanes', 'operating-points-v2', 'opp-table', 'pcie@0', 'perst-gpios', 'power-domains', 'ranges', 'wake-gpios' were unexpected)
	from schema $id: http://devicetree.org/schemas/pci/qcom,pcie-sm8550.yaml#

Thanks to Dmitry for suggesting the binding fix!

[1] https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
[2] https://lore.kernel.org/all/20251008-sm8750-v1-1-daeadfcae980@oss.qualcomm.com

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Changes in v2:
- Dropped the binding patch that got applied.
- Dropped mandating the WAKE# property in binding as not all platforms are
  supplying it and also fixed the binding example reported by the bot.
- Reworked the driver patch to just make 'phy' property as optional since all
  platforms specify PERST#.
- Link to v1: https://lore.kernel.org/r/20251010-pci-binding-v1-0-947c004b5699@oss.qualcomm.com

---
Manivannan Sadhasivam (2):
      dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties
      PCI: qcom: Treat PHY as optional for the new binding

 .../devicetree/bindings/pci/qcom,pcie-common.yaml        | 16 ++++++++++++++++
 .../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml       |  3 +++
 drivers/pci/controller/dwc/pcie-qcom.c                   |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)
---
base-commit: 5472d60c129f75282d94ae5ad072ee6dfb7c7246
change-id: 20251010-pci-binding-7d4d7799c6ed

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

[PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties

[Manivannan Sadhasivam]

Currently, the binding supports specifying the required PHY, PERST#
properties in two ways:

1. Controller node (deprecated)
	- phys
	- perst-gpios

2. Root Port node
	- phys
	- reset-gpios

But there is no check to make sure that the both variants are not mixed.
For instance, if the Controller node specifies 'phys', 'reset-gpios',
or if the Root Port node specifies 'phys', 'perst-gpios', then the driver
will fail as reported. Hence, enforce the check in the binding to catch
these issues.

It is also possible that DTs could have 'phys' property in Controller node
and 'reset-gpios' properties in the Root Port node. It will also be a
problem, but it is not possible to catch these cross-node issues in the
binding.

Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 .../devicetree/bindings/pci/qcom,pcie-common.yaml        | 16 ++++++++++++++++
 .../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml       |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
index ab2509ec1c4b40ac91a93033d1bab1b12c39362f..d56c0dc2ae4d3944294ca50cab708915c9f60ea8 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
@@ -111,6 +111,14 @@ patternProperties:
       phys:
         maxItems: 1
 
+    oneOf:
+      - required:
+          - phys
+          - reset-gpios
+      - properties:
+          phys: false
+          reset-gpios: false
+
     unevaluatedProperties: false
 
 required:
@@ -129,6 +137,14 @@ anyOf:
   - required:
       - msi-map
 
+oneOf:
+  - required:
+      - phys
+      - perst-gpios
+  - properties:
+      phys: false
+      perst-gpios: false
+
 allOf:
   - $ref: /schemas/pci/pci-host-bridge.yaml#
 
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
index 34a4d7b2c8459aeb615736f54c1971014adb205f..17abc7f7b7e9d71777380ddbfe90288e6187a827 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
@@ -77,6 +77,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-sc8180x.h>
+    #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/interconnect/qcom,sc8180x.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
 
@@ -164,5 +165,7 @@ examples:
 
             resets = <&gcc GCC_PCIE_0_BCR>;
             reset-names = "pci";
+
+            perst-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>;
         };
     };

-- 
2.48.1

Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties

[Krzysztof Kozlowski]

On Thu, Nov 06, 2025 at 04:57:16PM +0530, Manivannan Sadhasivam wrote:
> Currently, the binding supports specifying the required PHY, PERST#
> properties in two ways:
> 
> 1. Controller node (deprecated)
> 	- phys
> 	- perst-gpios
> 
> 2. Root Port node
> 	- phys
> 	- reset-gpios
> 
> But there is no check to make sure that the both variants are not mixed.
> For instance, if the Controller node specifies 'phys', 'reset-gpios',

Schema already does not allow it, unless I missed which schema defines
reset-gpios in controller node.

> or if the Root Port node specifies 'phys', 'perst-gpios', then the driver
> will fail as reported. Hence, enforce the check in the binding to catch
> these issues.

I do not see such check.

> 
> It is also possible that DTs could have 'phys' property in Controller node
> and 'reset-gpios' properties in the Root Port node. It will also be a
> problem, but it is not possible to catch these cross-node issues in the
> binding.

... so this commit changes nothing?

The commit actually does change, but something completely different than
you write here, so entire commit msg is describing entirely different
cast. What you achieve here is to require perst-gpios, if controller
node defined phys. Unfortunately your commit msg does not explain why
perst-gpios are now required...

> 
> Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

That's too many tags. Either someone reported you bug or someone
suggested you to do something, not both (and proposing solution is not
suggesting a commit since you already knew you need to make the commit
because of bug...)


> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-common.yaml        | 16 ++++++++++++++++
>  .../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml       |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> index ab2509ec1c4b40ac91a93033d1bab1b12c39362f..d56c0dc2ae4d3944294ca50cab708915c9f60ea8 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
> @@ -111,6 +111,14 @@ patternProperties:
>        phys:
>          maxItems: 1
>  
> +    oneOf:
> +      - required:
> +          - phys
> +          - reset-gpios
> +      - properties:
> +          phys: false
> +          reset-gpios: false
> +
>      unevaluatedProperties: false
>  
>  required:
> @@ -129,6 +137,14 @@ anyOf:
>    - required:
>        - msi-map
>  
> +oneOf:
> +  - required:
> +      - phys
> +      - perst-gpios
> +  - properties:
> +      phys: false
> +      perst-gpios: false
> +
>  allOf:
>    - $ref: /schemas/pci/pci-host-bridge.yaml#
>  
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> index 34a4d7b2c8459aeb615736f54c1971014adb205f..17abc7f7b7e9d71777380ddbfe90288e6187a827 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
> @@ -77,6 +77,7 @@ unevaluatedProperties: false
>  examples:
>    - |
>      #include <dt-bindings/clock/qcom,gcc-sc8180x.h>
> +    #include <dt-bindings/gpio/gpio.h>
>      #include <dt-bindings/interconnect/qcom,sc8180x.h>
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
> @@ -164,5 +165,7 @@ examples:
>  
>              resets = <&gcc GCC_PCIE_0_BCR>;
>              reset-names = "pci";
> +
> +            perst-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>;
>          };
>      };
> 
> -- 
> 2.48.1
> 

Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties

[Manivannan Sadhasivam]

On Sat, Nov 08, 2025 at 12:59:50PM +0100, Krzysztof Kozlowski wrote:
> On Thu, Nov 06, 2025 at 04:57:16PM +0530, Manivannan Sadhasivam wrote:
> > Currently, the binding supports specifying the required PHY, PERST#
> > properties in two ways:
> > 
> > 1. Controller node (deprecated)
> > 	- phys
> > 	- perst-gpios
> > 
> > 2. Root Port node
> > 	- phys
> > 	- reset-gpios
> > 
> > But there is no check to make sure that the both variants are not mixed.
> > For instance, if the Controller node specifies 'phys', 'reset-gpios',
> 
> Schema already does not allow it, unless I missed which schema defines
> reset-gpios in controller node.
> 

'reset-gpios' is currently a valid property for both controller and Root Port
nodes. Where does the schema restricts it?

> > or if the Root Port node specifies 'phys', 'perst-gpios', then the driver
> > will fail as reported. Hence, enforce the check in the binding to catch
> > these issues.
> 
> I do not see such check.
> 

Don't you think the below required properties not enforce this check for Root
Port and Controller node? This atleast makes sure that if 'phys' is present,
'reset-gpios' would be required for Root Port and 'perst-gpios' is required for
Controller node.

> > 
> > It is also possible that DTs could have 'phys' property in Controller node
> > and 'reset-gpios' properties in the Root Port node. It will also be a
> > problem, but it is not possible to catch these cross-node issues in the
> > binding.
> 
> ... so this commit changes nothing?
> 
> The commit actually does change, but something completely different than
> you write here, so entire commit msg is describing entirely different
> cast. What you achieve here is to require perst-gpios, if controller
> node defined phys. Unfortunately your commit msg does not explain why
> perst-gpios are now required...
> 

The Qcom PCIe controller node never supported 'reset-gpios' for PERST#. It used
the 'perst-gpios' property instead. And I do not wanted to replace it with
'reset-gpios' property since we had decided to move PERST# to Root Port node,
where 'reset-gpios' is already the norm.

> > 
> > Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> 
> That's too many tags. Either someone reported you bug or someone
> suggested you to do something, not both (and proposing solution is not
> suggesting a commit since you already knew you need to make the commit
> because of bug...)
> 

I disagree. Both Konrad and Krishna reported the issue in mixing up the
properties and driver ended up failing the probe. Then Dmitry suggested a schema
snippet [1] to catch these kind of mixups during DT validation. I did see it as
a valid suggestion that deserved the tag.

- Mani

[1] https://lore.kernel.org/linux-pci/qref5ooh6pl2sznf7iifrbric7hsap63ffbytkizdyrzt6mtqz@q5r27ho2sbq3/

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

[PATCH v2 2/2] PCI: qcom: Treat PHY as optional for the new binding

[Manivannan Sadhasivam]

Some platforms like the old ARM32 IPQ/APQ platforms do not supply the
'phys' property in devicetree. Hence, treat the PHY as optional in
qcom_pcie_parse_port(), so that they can work with the new binding model
of specifying PERST# in Root Port node.

Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 805edbbfe7eba496bc99ca82051dee43d240f359..53b41dc98f11ab953f7f4f31da642abf8b6faf83 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1723,7 +1723,7 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 
-	phy = devm_of_phy_get(dev, node, NULL);
+	phy = devm_of_phy_optional_get(dev, node, NULL);
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 

-- 
2.48.1

Re: [PATCH v2 2/2] PCI: qcom: Treat PHY as optional for the new binding

[Konrad Dybcio]

On 11/6/25 12:27 PM, Manivannan Sadhasivam wrote:
> Some platforms like the old ARM32 IPQ/APQ platforms do not supply the
> 'phys' property in devicetree. Hence, treat the PHY as optional in
> qcom_pcie_parse_port(), so that they can work with the new binding model
> of specifying PERST# in Root Port node.
> 
> Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---

Hm, I suppose they must rely on the PHY being pre-programmed then

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad